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

Provide a way to access raw headers #1437

Closed
blittle opened this issue May 12, 2022 · 6 comments
Closed

Provide a way to access raw headers #1437

blittle opened this issue May 12, 2022 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@blittle
Copy link

blittle commented May 12, 2022

This would solve...

As outlined at WHATWG/fetch there is often the need in a server environment to access cookie headers.

For example editing Cookie headers:

const h = new Headers;
h.append("Set-Cookie", "a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT");
h.append("Set-Cookie", "b=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT");

h.get("Set-Cookie")
// a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT, b=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT

Splitting the combined header value by , will give an invalid result. Instead getAll could be used to retrieve the individual headers.

The implementation should look like...

There are two solutions currently proposed:

  1. Node-fetch provides a non-standard Headers.prototype.raw method that returns entries for each (including duplicate) header.
  2. Re-introduce Headers.prototype.getAll to return multiple header values.

I have also considered...

  1. Not use undici primitives within NodeJS.
  2. Attempt to monkey patch undici

Additional context

  1. Cloudflare implemented .getAll().
  2. Node-fetch .raw() implementation
  3. Tracking at WinterCG
@blittle blittle added the enhancement New feature or request label May 12, 2022
@KhafraDev
Copy link
Member

KhafraDev commented May 12, 2022

The current headers implementation would need an overhaul to support this. It stores headers as strings, rather than arrays (which I believe goes against the spec) and doesn't concat values when using .get() (as, well, they're already strings).

I can see adding .getAll(), and like cloudflare, making it only work on set-cookie headers.

edit:
Unlike a header list, a Headers object cannot represent more than one Set-Cookie header. In a way this is problematic as unlike all other headers Set-Cookie headers cannot be combined, but since Set-Cookie headers are not exposed to client-side JavaScript this is deemed an acceptable compromise. Implementations could choose the more efficient Headers object representation even for a header list, as long as they also support an associated data structure for Set-Cookie headers.

It looks like undici only needs to store set-cookie headers as arrays and instead allowing .getAll('set-cookie').

@ksmithut
Copy link

ksmithut commented Jun 5, 2022

Some more additional context, it looks like Deno's implementation is also a bit different:

Deno:

Deno 1.22.2
> Array.from(new Headers([['set-cookie', 'a=b'], ['set-cookie', 'b=c']]))
[ [ "set-cookie", "a=b" ], [ "set-cookie", "b=c" ] ]

Node:

Welcome to Node.js v18.3.0.
> Array.from(new Headers([['set-cookie', 'a=b'], ['set-cookie', 'b=c']]))
[ [ 'set-cookie', 'a=b, b=c' ] ]

Not sure what the "right" thing to do is, just pointing out an implementation detail.

I've been looking at creating an http server library that translates node's http.IncomingMessage to a Request object, and in turn takes a Response object and translates that to node's http.ServerResponse (similar to how Deno's http server works). Perhaps this is a fools errand as it seems the fetch API was not meant to be used in a Server context.

@KhafraDev
Copy link
Member

Undici is the spec-compliant way, Deno purposefully doesn't concatenate set-cookie headers.

https://github.com/denoland/deno/blob/bb3387de17953ea1f0558f5f9863b0cd6c59d48c/ext/fetch/20_headers.js#L212-L215

@ksmithut
Copy link

ksmithut commented Jun 5, 2022

Right, they seem to do that because they reuse the Request and Response classes in a server context, not just for usage with fetch. If that's not a goal of undici, then it should probably just stick with the spec.

@KhafraDev
Copy link
Member

KhafraDev commented Jun 5, 2022

Headers.prototype.getSetCookie will solve this use case. I don't think another solution would be accepted unless something is decided in the wintercg fetch group.

Deno most likely does this just because it's better for the user; otherwise the array of set-cookie headers wouldn't be exposed.

@sbellone
Copy link

I think this can be closed now that .getSetCookie() has been introduced: #1915

@ronag ronag closed this as completed Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants