-
-
Notifications
You must be signed in to change notification settings - Fork 31
feat(axios-to-whatwg-fetch): introduce
#269
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
base: main
Are you sure you want to change the base?
Conversation
|
omg please yes |
|
cc @nodejs/userland-migrations can I have first review on this one |
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.
This is a great initial implementation 🙂
I think it would be better to consolidate the config handling into a helper since all axios methods support it.
Blocker: There are some axios config options that are not supported here, and, if present in userland code, the absence of support will surely break that userland code. For instance:
transformResponse I think that can probably just be converted to an initial then() prepended to the outputted fetch?
transformRequest I think this can be handled by merely setting fetch's init.body like body: transformRequest(bodyExpression). That would probably also work for paramsSerializer.
If we don't want to handle those more exotic axios config options initially, I think that's okay, but the migration needs to detect them and abort.
| }, | ||
| }, | ||
| { | ||
| oldBind: '$.request', |
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.
Let's include the axios.request or its location (file.ext:row:col) in the warning output here so the user knows what triggered.
Also, I think maybe these should be console.error instead of warning because part of the migration failed.
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.
Let's include the axios.request or its location (file.ext:row:col) in the warning output here so the user knows what triggered.
I didn't get this
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.
Which part don't you get?
Co-authored-by: Jacob Smith <[email protected]>
Co-authored-by: Jacob Smith <[email protected]>
Co-authored-by: Jacob Smith <[email protected]>
Co-authored-by: Jacob Smith <[email protected]>
Co-authored-by: Jacob Smith <[email protected]>
|
@JakobJingleheimer I tried to apply all of your proposal. But why the ci didn't ran ? |
|
It looks like CI did run? If not, was this whilst I was fiddling with the repo permissions? (I specifically told you on slack you'd temporarily have |
it's ran after the merge commit. idk what really happened but it's work now ! it's didn't help since I dunno why windows is doing windows thing |
JakobJingleheimer
left a comment
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.
Great tidy!
It's still missing handling for transformRequest, transformResponse, paramsSerializer, etc, which was the blocker before. It needs to either
- support them
- detect them and abort/fail the migration
Doing neither will result in broken userland code.
Related issue
close #191