[WIP] attempt upgrade to axios 1.x #213
Conversation
|
AFAICT the travis failures here are caused by |
|
@ricellis this looks great, I appreciate all of the effort and detailed documentation. I should've been doing the same, as I've done a lot of this locally without any updates or notes on changes. However, it's a good confirmation on everything I'd worked on and you've also solved some things that were still on my to-do list. I'll go through your PR today and think about the dependency issue with the retry tests and get back to you. I also need to do some tests for compatibility with generated code and see if any of this constitutes breaking changes for the core. This is looking great overall though, I'd love to try and merge it in the next couple of days. |
Happy to hear that it's on the same lines, do cherry-pick or copy/paste as needed into your work. We can close this PR down np.
With any luck a non-alpha 1.2.0 will happen and make this problem go away. I think anything using retries will break without the type fixes in that version (not just the tests).
Yeah I wasn't sure whether we could get away with dropping the boolean on the I'm not entirely sure on side-effects of the typescript version upgrade - I was definitely just focussed on getting axios working. I do think this might not work well with Node 12, but that is out of support now anyway. If the engine does move then I'd also advocate for updating the version of the |
I think I'd rather just keep your PR - it's in better shape than mine!
I think you're right. I ran into this problem as well. We can monitor for a release but the timeline might not be as soon as we'd like.
I think I'd just dropped it in my local changes but I prefer what you've done here 👍
This is what I'm most concerned about but I had done some tests on this subject already and things seemed like they should be fine. I just want to double check.
Yep, we have an issue on our internal board to move up the engines and update our Travis builds. We need to do that soon. Agreed that we should bump the types when we do that. |
|
Just to add another note here on axios 1.2 and the retry issues. |
ricellis
force-pushed
the
ricellis/wip-axios-1.x
branch
from
2e4618b
to
446b1a3
Compare
|
Axios 1.2.0 is out now so I've rebased using that instead of the I pushed an additional commit to move to 14 minimum and a test matrix of 14 & 16 and it looks ok now. |
This also requires new versions of: * `typescript` - to support the axios 1.x types * `jest` - to support the axios 1.x module * `axios-cookiejar-support` for axios 1.x module compatibility Note: 1.2.0 has fixes to axios request headers that otherwise mean the retry tests fail. Signed-off-by: Rich Ellis <ricellis@users.noreply.github.com>
Use `paramsSerializer` legacy compatibility `serialize` function with existing stringify function. Correct axios-cookiejar-support import and intialization. Provide backwards compatibility for `jar: true` option. Correct mocking in request-wrapper.test.js. Remove `withCredentials` from cookiejar.test.js. Set `synchronous: true` on mock cookie jar. Add new test for usage of real tough-cookie CookieJar. Signed-off-by: Rich Ellis <ricellis@users.noreply.github.com>
build-request-file-object.test.js and get-content-type.test.js both make use of streams with non-existent paths to check errors. Possibly since upgrading jest these cause suites to fail with e.g.
```
Error: ENOENT: no such file or directory, open '/fake/path/custom-name.env'
Emitted 'error' event on ReadStream instance at:
at emitErrorNT (node:internal/streams/destroy:157:8)
at emitErrorCloseNT (node:internal/streams/destroy:122:3)
at processTicksAndRejections (node:internal/process/task_queues:83:21) {
errno: -2,
code: 'ENOENT',
syscall: 'open',
path: '/fake/path/custom-name.env'
```
Since the errors are expected for these tests add a no-op handler to handle the error events.
Signed-off-by: Rich Ellis <ricellis@users.noreply.github.com>
Signed-off-by: Rich Ellis <ricellis@users.noreply.github.com>
ricellis
force-pushed
the
ricellis/wip-axios-1.x
branch
from
a7d11f9
to
db281ed
Compare
|
@ricellis Awesome - this looks great. Thanks again for all of your work on this. I will do a little testing to see how this plays with SDKs and ensure the changes are not "breaking" but I'd like to merge this soon |
|
@dpopp07 is there a way to publish a snapshot build we could test too? |
|
@ricellis Yes, that's a great idea. I'll do that today |
|
@ricellis This branch can now be installed with the "beta" tag ( |
|
@ricellis based on my testing, everything works fine but SDK developers will need to update the |
Thanks, this makes testing things a lot easier.
Our SDK looks ok, but seeing some problems in other downstream projects (specifically around
We run dependabot so we've been bumping our downstream dependencies regularly and they were way ahead of what was used here so I probably didn't think about this hard enough. I suppose it may be possible to find a TS version that works that isn't as big a leap since I just moved to latest I didn't probe around to see what the minimum usable version might be. Although I suspect it will have to be a 4.x given there appeared to be a syntax change of some kind and once 3 to 4 has been crosssed there probably isn't much to be gained from older minor versions. I don't know much about the compatibility statement between 3.x and 4.x and whether transpiling at 4.x rules out using with 3.x or whether it specifically depends on whether new features are used. I guess the break seen indicates that axios at least uses some of those 4.x features, but whether it impacts the parts of axios the core exposes could make a difference I suppose.
Yes, as above I don't know a lot abot the requirements here, but it is entirely possible that projects relying on TS 3.x will need to update to 4.x to work with types transpiled at 4.x. |
AFAICT there is an incompatibility between newer versions of
|
|
@dpopp07 I pushed another branch with a commit 220c48e that removes the dependency on Given the availability of interceptors in axios I was looking for options that avoid the dependency that is giving us trouble. Although the change is probably a bit rough around the edges, especially in terms of validating it works correctly when setting/getting multiple cookies for a single URL. I'm not sure how many other SDKs rely on cookie support, but if you want to consider making another beta from that branch/commit then I'd be happy to run our test pipelines and see if it works better for us. |
I don't think that many do. I'll push a second beta with your changes shortly Edit: done, beta tag updated |
|
@dpopp07 thanks for that. The bad news is that I see a bunch of different failures using the Anyway the "new" failures look like some kind of unicode or encoding problem - I'll invest some time into those and see if I can move this any closer to stability, but for now it is sadly too broken to proceed. |
|
OK so the problem is axios/axios#5296 which is fixed in axios |
|
@ricellis I updated the beta with your new changes |
|
Thanks @dpopp07 - I couldn't see which commit made the Please could you make one from 0f65ae1? I think that has the best chance of success. If we're still having issues after that I think we'll have to think of some kind of alternative or potentially wait for axios 1.x to stabilize further. |
Yep, I see what happened. My mistake. I just published the new beta from that commit |
No worries, I appreciate the time you've been putting into releasing all these betas to help me test downstream and I didn't make it any easier with my own mistakes and having two separate branches!
|
|
Closing this, replaced by:
|
An attempt to upgrade to axios 1.x.
Commit summary:
package.jsondepenenciestypescript- to support the axios 1.x typesjest- to support the axios 1.x moduleaxios-cookiejar-supportfor axios 1.x module compatibilityparamsSerializerlegacy compatibilityserializefunction with existing stringify function.jar: trueoption.withCredentialsfrom cookiejar.test.js.synchronous: trueon mock cookie jar.Checklist
npm testpasses (tip:npm run lint-fixcan correct most style issues)