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

Get multiple headers as an aray rather than a combined value? #506

Closed
nfriedly opened this issue Mar 13, 2017 · 14 comments
Closed

Get multiple headers as an aray rather than a combined value? #506

nfriedly opened this issue Mar 13, 2017 · 14 comments

Comments

@nfriedly
Copy link

Hi,

I have a set-cookie-parser library and I got a report that it fails when parsing fetch responses that contain multiple Set-Cookie headers. Looking through the spec and the output, I see that it's returning a combined value for both headers.get() and the now-deprecated headers.getAll().

IMHO, getAll() should return an array, but if it's too late for that then perhaps a new method could be added that returned an array, or alternatively an extra parameter to headers.get() to control it's output? (Or is it already possible and I'm just missing something?)

@nfriedly
Copy link
Author

Actually, it looks like a for/of loop gets each header individually, so while a little bit awkward that solves my issue. Now I need to figure out how he's actually getting the set-cookie headers in the first place since it appears that fetch() blocks those.

@domenic
Copy link
Member

domenic commented Mar 14, 2017

Actually, it looks like a for/of loop gets each header individually

That would be a bug in whatever browser you're testing. Can you let us know which one that is so we can file an issue and write tests to get it fixed?

@domenic domenic reopened this Mar 14, 2017
@sshymko
Copy link

sshymko commented Mar 14, 2017

Hi,

Although getAll() is deprecated, it must return an array of individual values, right? Based on that assumption, I submitted a bug #12846 to React Native. Currently, their platform-specific implementation of the Fetch API (at least iOS) returns a single array item = all values combined.

@annevk
Copy link
Member

annevk commented Mar 14, 2017

getAll() shouldn't exist. It's more than deprecated, it's removed. Also, cookie headers are not supported whatsoever. They're excluded from the API per https://fetch.spec.whatwg.org/#forbidden-response-header-name.

@sshymko
Copy link

sshymko commented Mar 14, 2017

@annevk
So what should to be used in mobile environments running JavaScript such as React Native than?

@annevk
Copy link
Member

annevk commented Mar 14, 2017

I don't know. I think if we actually had to expose cookies properly they'd have to be in some kind of side-table due to the weird way they parse.

@nfriedly
Copy link
Author

nfriedly commented Mar 14, 2017

Actually, it looks like a for/of loop gets each header individually

That would be a bug in whatever browser you're testing. Can you let us know which one that is so we can file an issue and write tests to get it fixed?

This is in Firefox 52, although it apparently only works when I create the Headers object myself. Ones from the server concatenate the string even in for/of loops, as does Chrome. (Both for headers they allow, such as X-Foo. No browser gives access to Set-Cookie headers, although apparently the fetch implementation in React Native does.)

To clarify, this code logs two entries in Firefox but only one combined entry in Chrome:

headers = new Headers([['X-Foo', 'a=b, and c'],['X-Foo',' d=e']])
for(let header of headers) {
  console.log(header);
}

http://jsbin.com/gevixekimo/edit?js,console

I closed this issue thinking that the Firefox behavior would be everywhere. Reading the spec more thoroughly, I see that it prescribes the Chrome behavior. In my opinion the Firefox version is the more useful behavior and the spec should be changed to match it.

@annevk
Copy link
Member

annevk commented Mar 14, 2017

I don't think we'll be making that change. We aligned on this definition in #189 quite a while ago and I don't really see what's wrong with it (other than not working for cookies, which we knew at the time).

I will add a test for that though and file a bug against Firefox in due course.

@annevk
Copy link
Member

annevk commented Mar 14, 2017

(Unless I can get a volunteer? 😊)

@nfriedly
Copy link
Author

nfriedly commented Mar 14, 2017

Ok, so with some more reading I'll concede that you do have the HTTP RFC on your side, and this apparently does work for everything that's RFC-compliant except for cookies.

I'm not giving up on the idea entirely, but that does take a bit of the argument out of my sails.

What would volunteering look like? Just re-write the spec as I see fit and send a PR?

Also, Node.js follows the combining behavior except for set-cookie headers, and this behavior falls through to node-fetch. Perhaps we need to ask React Native to do the same for their fetch implementation.

@annevk
Copy link
Member

annevk commented Mar 14, 2017

The volunteering is for tests and filing the Firefox bug. I don't see reason to revisit the standard itself.

@annevk
Copy link
Member

annevk commented Sep 5, 2017

@annevk annevk closed this as completed Sep 5, 2017
Ms2ger pushed a commit to web-platform-tests/wpt that referenced this issue Oct 4, 2017
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this issue Nov 16, 2017
@nfriedly
Copy link
Author

nfriedly commented Jun 8, 2018

I know this is an old ticket, but for anyone who's interested, we now have a good solution to the problem. Thanks to a PR from @chrusart, the set-cookie-parser library is now capable of splitting the combined cookie string! They can then be parsed as individual set-cookie headers. See https://www.npmjs.com/package/set-cookie-parser#usage-in-react-native

Please test it out and let me know how it works for you.

@annevk
Copy link
Member

annevk commented Nov 29, 2019

I'm reconsidering this request in #973 as we do have a guard of "none" so there is a way to work with Set-Cookie in this API, except that it breaks down as described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants