-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
On Hold: Update Axios to latest version: 1.6.2 #338
Conversation
I would vote for dropping support for the very old NodeJS versions (I am on Node 18). I really want my payment provider to not have CVE's... Release a version 4 so that it is clear this is a major upgrade. |
@maria-swierblewska Can you look into the numbers a bit and then come up with a recommendation around dropping version support? |
Hello @janpaepke I've done some analysis on past year's data on Node versions used versus the volume of payments they support and it is clear that the support should be maintained for v14 and higher (v14 supports almost 30% of all volume). In the past year all versions under v14 account for 2.94% of all payments volume so indeed we think there is no need to support the versions below 14. |
@maria-swierblewska thank you for your info. @fjbender |
@janpaepke for the sake of transparency we'd suggest to proceed with a separate branch with the v14 update and rebasing this one |
Added the node update here: #341 This should be pretty straight forward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Went over all changes with @janpaepke and tests are passing now.
Thanks for all the work! When will this be released? |
UPDATE
It seems this PR fails with node 6.14. See discussion below for more info.
This PR updates the Axios dependency to 1.6.2, since the version currently in use (0.27.2) contains a reported vulnerability (Cross-Site Request Forgery).
Even though this is actually of no consequence in the context of the usage of this library, this results in warning messages for all consumers, who will have to then research if a risk exists.
This resolves: #337
The update is technically pretty straight forward, since there appears to be no breaking change in behaviour within the scope of this library.
The only changes to the library are type related.
Most of the work spent on this PR was getting the tests to work.
Since changes to the tests are obviously a delicate matter, I broke the updates down into individual commits for better reviewability:
This includes the version update of axios and the only necessary changes to get the build to compile. As mentioned before, these are only type related.
This contains three fairly uncontroversial changes:
There are no snapshot changes other than the removal of the prototype names.
fail
method, even though DefinitelyTyped still thinks so. This can be easily fixed by importing the fail method fromnode:assert
, which has the same behavior in our context.jest.useFakeTimers()
.I suspect the reason to be a changed logic for their internal request timeout management.
As a consequence the initial request would not be processed, when simply awaiting
tick ()
. What I did find to work was usingawait jest.advanceTimersByTimeAsync(0);
instead, which should have the same effect.Additionally this is how we awaited subsequent requests:
jest.advanceTimersByTime(2e3); await tick();
The only method I found to work reliably now was this:
await tick(); await jest.advanceTimersByTimeAsync(2e3);
Swapping the order and making the timer async should still not change the intention of this test.
In conclusion, this should be an appropriate upgrade with no breaking changes for the lib.
The only part that might deserve a closer look are the changes describe in #4 above.