Skip to content

Case insensitive handling of headers#1291

Closed
betalb wants to merge 2 commits intoaxios:masterfrom
betalb:fix-1237
Closed

Case insensitive handling of headers#1291
betalb wants to merge 2 commits intoaxios:masterfrom
betalb:fix-1237

Conversation

@betalb
Copy link
Copy Markdown

@betalb betalb commented Jan 14, 2018

Fixes #1237

One breaking change was introduced to internal util merge method: it merges objects ignoring case of keys. This change is covered with tests to highlight new behaviour.

This was a trade-off to minimize impact on bundle size. Currently this method is used to merge configuration objects, so I don't see any issues with this change.

@betalb
Copy link
Copy Markdown
Author

betalb commented Jan 14, 2018

Just realized that a lot of people are modifying instance.defaults.headers by reference.

In this case above PR won't fix the issue to full extent, result sent by adapters make result won't be fully predictable

IMHO modification of configuration (headers in particular) by reference doesn't seem right. Better place may be transformRequest.

But pure functions look like a cleaner approach -- do not allow to mutate anything, require always to produce new object in interceptors, transformers and freeze defaults in DEV mode

@rootsher
Copy link
Copy Markdown

@betalb I think the same about cleaner approach, but some people (as with the rest you wrote) needs modify instance.defaults.headers by reference. If we would like to merge this pull request, please also respectively update version of axios and describe this "inconvenience" for mutating programmers ;)

@emilyemorehouse emilyemorehouse force-pushed the master branch 2 times, most recently from 2760755 to 48a7902 Compare February 19, 2018 23:22
@codeclown
Copy link
Copy Markdown
Contributor

I'm not a huge fan of introducing a new class for this purpose, it seems too bloaty for the codebase. I think it would be best to convert all header names to lowercase at all times, which would remove the need for any special logic related to this.

@betalb
Copy link
Copy Markdown
Author

betalb commented Oct 22, 2018

I'm not closely following current development effort, but remember that there was talk about reworking way how configuration is done. Especially default configuration.

I.e. at the time of writhing, there was a way to directly mutate default headers, inherited by all instances that means by the time we receive headers in adapter we already may have ambiguity.

Note that my PR also doesn't address this issue

@codeclown
Copy link
Copy Markdown
Contributor

The config situation was addressed a while ago (#1395) and it shouldn't be possible to mutate defaults to instances after they have been set. However, you make a good point which I did not initially consider.

@jasonsaayman
Copy link
Copy Markdown
Member

Hi,

Thank you for this pull request, this is a pretty stale pull request so I am going to close this in favour of #2880 as its a bit fresher and seems to solve the asme issue. If you feel it does not solve this issue please open an issue again.

Thanks

@axios axios locked and limited conversation to collaborators May 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Headers should be compared with case ignored

4 participants