Skip to content

Conversation

@AugustinMauroy
Copy link
Member

@AugustinMauroy AugustinMauroy commented Nov 8, 2025

Related issue

close #191

@AugustinMauroy AugustinMauroy added the awaiting reviewer Author has responded and needs action from the reviewer label Nov 8, 2025
@AugustinMauroy AugustinMauroy requested review from a team and removed request for a team November 8, 2025 13:49
@AugustinMauroy AugustinMauroy removed the awaiting reviewer Author has responded and needs action from the reviewer label Nov 8, 2025
@AugustinMauroy AugustinMauroy marked this pull request as draft November 8, 2025 13:50
@JakobJingleheimer
Copy link
Member

omg please yes

@AugustinMauroy AugustinMauroy requested a review from a team December 10, 2025 10:34
@AugustinMauroy
Copy link
Member Author

cc @nodejs/userland-migrations can I have first review on this one

@AugustinMauroy AugustinMauroy added the awaiting reviewer Author has responded and needs action from the reviewer label Dec 13, 2025
@brunocroh brunocroh requested a review from a team December 19, 2025 22:19
@brunocroh brunocroh requested a review from a team January 9, 2026 09:43
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a 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',
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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?

@JakobJingleheimer JakobJingleheimer added awaiting author Reviewer has requested something from the author and removed awaiting reviewer Author has responded and needs action from the reviewer labels Jan 17, 2026
@JakobJingleheimer JakobJingleheimer moved this to 🏗 In progress in Userland Migrations Board Jan 17, 2026
@AugustinMauroy
Copy link
Member Author

@JakobJingleheimer I tried to apply all of your proposal.

But why the ci didn't ran ?

@JakobJingleheimer
Copy link
Member

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 read-only permissions for a bit, which means you couldn't do anything other than read within the repo)

@AugustinMauroy
Copy link
Member Author

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 read-only permissions for a bit, which means you couldn't do anything other than read within the repo)

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

@AugustinMauroy AugustinMauroy added awaiting reviewer Author has responded and needs action from the reviewer and removed awaiting author Reviewer has requested something from the author labels Jan 17, 2026
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a 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.

@JakobJingleheimer JakobJingleheimer added awaiting author Reviewer has requested something from the author and removed awaiting reviewer Author has responded and needs action from the reviewer labels Jan 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting author Reviewer has requested something from the author

Projects

Status: 🏗 In progress

Development

Successfully merging this pull request may close these issues.

feat(axios-to-whatwg-fetch)

4 participants