Skip to content
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

fix: support existing header instance as argument #5090

Closed
wants to merge 5 commits into from

Conversation

lohart13
Copy link

When an existing axios config is passed to the axios function, the headers are not read correctly and also the default headers don't get merged correctly therefore it is easier to get the default headers from the current instance of Axios.

This closes #5089.

@KrayzeeKev
Copy link

What is the process for fixes like this to get merged and patch releases to come out? I've got code scanning security software complaining that I'm running a very old version of Axios (0.27) and I need to provide some guidance on when I'll adopt the obviously much more secure version 1.x :-)

@paulsouche
Copy link

paulsouche commented Oct 18, 2022

Hello ! I am not contributor here and I already approved this change.

I hope this will get fixed at some points because axios-retry retrying the request with the config containing AxiosHeaders is broken

@KrayzeeKev
Copy link

Hello ! I am not contributor here and I already approved this change.

I hope this will get fixed at some points because axios-retry retrying the request with the config containing AxiosHeaders is broken

Looks like something happened that requires yet another approval.

@KrayzeeKev
Copy link

@paulsouche It seems it wants you to approve again. Are you able? I'm so keen to get this one rolled in and released. I'm currently facing a serious choice of having to drop Axios altogether as I can't justify running a v0 release when a v1.1 already exists.

@lohart13
Copy link
Author

@DigitalBrainJS does #5169 resolve this PR?

@jasonsaayman
Copy link
Member

Going to close this one in favour of #5162

@DigitalBrainJS
Copy link
Collaborator

DigitalBrainJS commented Oct 30, 2022

@lohart13 No. PR #5169 does not add support for AxiosHeaders instance to be used as Axios header config but takes the first step in this direction. The implementation needs to go a bit deeper than just adding an assertion in one place and seems this PR doesn't solve the problem globally but adding a new config merge bug since completely ignores context headers.
Since config merging is a core util, we must carefully approach the search for the best solution, since an unsuccessful solution will affect absolutely all Axios functions and may cause significant regression bugs.
This will be fixed in a separate PR, by enhancing the mergeConfig util implementation.

Copy link

@MarkCupitt MarkCupitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested this on Axios 1.1.3 by patching the files and it solves the problem

driiftkiing added a commit to tokyodrift1993/YouVersion that referenced this pull request Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Axios config headers causes requests to fail with ERR_INVALID_CHAR
6 participants