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

Allow appending multiple Set-Cookie Headers #1851

Closed
nnelgxorz opened this issue Oct 22, 2022 · 4 comments · Fixed by #1864
Closed

Allow appending multiple Set-Cookie Headers #1851

nnelgxorz opened this issue Oct 22, 2022 · 4 comments · Fixed by #1864
Labels
STATUS-1: needs triage New issue which needs to be triaged TYPE: enhancement New feature or request

Comments

@nnelgxorz
Copy link
Contributor

Is your feature request related to a problem?

The current headers implementation uses an append function that concatenates header values together with a ", ". This works for all headers except Set-Cookie.

Describe the solution you'd like

There are a few options I can think of:

  • create special functions for each headers method specifically for cookies: getCookie, setCookie, appendCookie.
  • keep the existing headers methods, but check for the header value internally and store cookies separately. (if (name === 'Set-Cookie) {...}`
  • keep everything the same internally, but special case Set-Cookie to values on the ", " before creating the response and generating valid Set-Cookie headers for each value.
  • same as above, but use node-fetch's .raw() to get the Set-Cookie headers as an array.

Describe alternatives you've considered

cookie

Additional context

whatwg/fetch#973 - Discussion around formalizing a solution for Set-Headers
https://github.com/withastro/astro/pull/3092/files - Astro PR using the adding multiple Set-Cookies for Netlify
https://github.com/sveltejs/kit/pull/3502/files - Svelte PR adding multiple Set-Cookies

@nnelgxorz nnelgxorz added TYPE: enhancement New feature or request STATUS-1: needs triage New issue which needs to be triaged labels Oct 22, 2022
@manucorporat
Copy link
Contributor

I think good cookie support is super important, wanna take into preparing a PR most likely inspired by sveltekit and astro?

This would be such a massive improvement!!

@nnelgxorz
Copy link
Contributor Author

Definitely! I was going to take it on yesterday, but it looks like Qwik City e2e tests are broken. I'm guessing because of the refactor Adam did on the middlewares/servers.

@nnelgxorz
Copy link
Contributor Author

Opened a PR at #1864

On paper, I think this would fix the issue, but without being able to run tests, I don't feel comfortable merging yet.

@nnelgxorz
Copy link
Contributor Author

PR is ready for review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
STATUS-1: needs triage New issue which needs to be triaged TYPE: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants