feat(push): Add 'signal' parameter#1872
Conversation
|
The tests are failing because |
In older versions of NodeJS, e.g., 12, AbortController is not defined.
|
Yeah, I see now that the tests are being executed with Node 12. Is there a way to specify the version of Node when running the tests? Because, right now don't test the new functionality, we test if everything still works with Node 12. |
|
If you can remove Node 12 and make Node 16 the lowest one that is support, that will be ok. It's probably also a good idea to add a newer version of Node as well. If you want to contribute with update, you're more than welcome. But I suggest creating another PR with just the Node update and rebase this one, when node 12 is removed on main. This will make sure that tests are working correctly and if there are any tests that are failing we will know it's not the problem with abort Controller. |
|
I sent another PR: #1880 |
|
It looks like now the only errors came from HTTP 401. |
|
yeah |
|
Any update on this? |
|
There are conflicts, the tests should work now. The author needs to merge and resolve conflicts. |
3 participants
I'm adding a
signalparameter to an existing commandpushThis pull request is related to this question: #1867
signalparameter to the function insrc/api/push.js(andsrc/commands/push.js)__tests__/test-push.jsnpm run add-contributorPlease, bear with me, that's the first time I see the code :)
I tried to follow the coding style and patterns I saw. Not sure about the right documentation of
signalparam. Since it is optional, I would expect to be written like@param {AbortSignal?} [args.signal], but I saw the other optional params weren't written like this.Also, please let me know if there were other
httpcalls, where thesignalparam has to be added.Please, feel free to to edit the PR directly, this will save us time.
If this pattern is OK, adding to the other commands shouldn't be too hard, I think.
Best regards,