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
Support header name capitalization #9891
Support header name capitalization #9891
Conversation
Updates the headersToLowerCase function to allow for intentionally capitalized header names, hopefully without breaking servers that expect our header names to be all-lowercase and hopefully still preventing people from accidentally duplicating headers
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.
Thanks very much for working on this @MrDoomBringer! You rightfully called this out in your PR description:
Because some (non-http-spec-compliant) servers might be expecting header names from us to be lowercase, I'm PRing against
release-3.7
instead ofmain
.
I think we would need to go further here however, as this change could be breaking. If people currently have headers defined in their application using upper case notation of some kind, we're converting those headers to all lower case and firing them off to their server of choice. If we ship a minor/patch version of Apollo Client that retains their mixed/upper case however, there is a chance their server can't handle non-standard header case, and fail. This means we have 2 options:
- Retarget these changes against Apollo Client 4.0. Or ...
- We make the conversion of headers to lower case configurable via an
HttpLink
option. This conversion would remain on by default, but people that really need header case preserved could set apreserveHeaderCase
option to true in theHttpLink
constructor, and we'll adjust the conversion internally based on this.
Option 2 is probably our best bet if we want to get this resolved sooner than later (as we could ship this in 3.7), but that means adding/supporting another HttpLink
option.
@benjamn @alessbell I'd love to hear your thoughts as well. Thanks!
(Accidental debugger statements count as style right?)
Thanks a ton for the review @hwillson ! I went ahead and made a feature commit (76655a3) adding the Y'all let me know if we think this is a good route forward and I'll add additional testing & documentation :-) |
Closes #6741
Header names will now preserve their capitalization when passed into
setContext()
. This allows for non-http-spec-compliant servers to use capitalized header names, etc.Originally the
headersToLowerCase
function was created to prevent unintentionally duplicating a header instead of overwriting it (see #8447 and #8449). This PR adds an additional loop to that function which, after removing duplicates, returns header names to their original capitalization, hopefully preventing regression on #8447 while still closing #6741.Because some (non-http-spec-compliant) servers might be expecting header names from us to be lowercase, I'm PRing against
release-3.7
instead ofmain
.Checklist: