feat(push): Add 'signal' parameter

[ipeychev]

I'm adding a signal parameter to an existing command push

This pull request is related to this question: #1867

• added signal parameter to the function in src/api/push.js (and src/commands/push.js)
• documented the parameter in the JSDoc comment above the function
• added a test case in __tests__/test-push.js
• Ran npm run add-contributor
• squashed the PR and added the commit message "feat(push): Added 'signal' parameter"

Please, 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 signal param. 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 http calls, where the signal param 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,

[jcubic]

The tests are failing because AbortController is not defined in one of the testing environments:

https://dev.azure.com/isomorphic-git/isomorphic-git/_build/results?buildId=3318&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=5b4cc83a-7bb0-5664-5bb1-588f7e4dc05b

[ipeychev]

Yeah, I see now that the tests are being executed with Node 12. AbortController was introduced in Node 15.
I updated the test to take into account this situation and the test is passing now.

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.

[jcubic]

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.

[ipeychev]

I sent another PR: #1880
There is more than one commit - I needed to downgrade some packages, but now the tests are passing on my Mac and on Azure/Linux. Of course, there were some broken tests before, they are still failing, but this is another story.

[jcubic]

It looks like now the only errors came from HTTP 401.

[ipeychev]

yeah