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

Support Set-Cookie headers in endpoints #4375

Closed
rgossiaux opened this issue Mar 17, 2022 · 5 comments
Closed

Support Set-Cookie headers in endpoints #4375

rgossiaux opened this issue Mar 17, 2022 · 5 comments
Milestone

Comments

@rgossiaux
Copy link

Describe the problem

If your endpoint returns a Headers object that contains Set-Cookie header(s), you get an error:

if (headers instanceof Headers) {
if (headers.has('set-cookie')) {
throw new Error(
'Endpoint request handler cannot use Headers interface with Set-Cookie headers'
);
}

because, as was pointed out in the PR adding this, dealing with Set-Cookie headers is a PITA, and headers.get("Set-Cookie") returns some monstrosity

Describe the proposed solution

Since Kit is using node-fetch v3, if headers is the output of a fetch, you can call headers.getAll("Set-Cookie") to get a nice array of Set-Cookie headers. Instead of erroring, it would be nice if Kit could just do this automatically--that way, as a user you can make a fetch inside your endpoint and simply pass along the result's headers object without having to deal with this manually.

Note that there's an open PR that might remove this getAll method in node-fetch v4: node-fetch/node-fetch#1484 In this case, the implementation would be changed to use this package: https://www.npmjs.com/package/fetch-headers which also provides a way of extracting multiple Set-Cookie headers--when iterating over the headers object, each Set-Cookie will appear as its own header. Details are on the npm page.

Alternatives considered

Users can use headers.getAll themselves to manually forward the Set-Cookie headers:

let response = await fetch(someUrl);
let cookies = response.headers.getAll("set-cookie");
return {
  headers: {
    "set-cookie": cookies,
  },
};

This works fine but isn't very discoverable & could break if SvelteKit updates its node-fetch version.

Importance

would make my life easier

Additional Information

No response

@mrkishi
Copy link
Member

mrkishi commented Mar 18, 2022

Yeah, this is annoying, especially considering not all adapters use the node-fetch polyfill. I don't have any ideas other than requiring adapters to provide a polyfill with a getAll() implementation or making sure the platform does (such as Cloudflare).

@Rich-Harris
Copy link
Member

Given that different platforms workaround this issue in different ways, there's really nothing we can do — we can't monkey-patch the Headers provided by Cloudflare and Deno to act the same way. We must direct our anger at the people responsible for the relevant standards, who say things like "the spec is written for browser environments" and dismiss good faith attempts to come up with solutions.

Unless and until consensus among platforms is reached, it will continue to be necessary to return a POJO with an array for set-cookie, I'm afraid.

@mrkishi
Copy link
Member

mrkishi commented Mar 18, 2022

We wouldn't need to monkey-patch the platform's implementation. When receiving one, we could instead wrap them in the adapter-specific polyfill. Alternatively, we could require and use a standalone getAll() or getSetCookie() from adapters.

Granted, it'd still be annoying, but doable, no?

@Rich-Harris Rich-Harris reopened this Mar 18, 2022
@Rich-Harris
Copy link
Member

Well, we'd need to replace the global Headers with our own thing. Assuming that's even possible in all environments, it would involve a lot of nastiness and would probably be a bug farm. But I've reopened this in case you think there's a sensible route forward

@Rich-Harris
Copy link
Member

Closing as this stuff is now handled by event.cookies

@Rich-Harris Rich-Harris closed this as not planned Won't fix, can't repro, duplicate, stale Nov 20, 2022
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

No branches or pull requests

3 participants