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

Feature request: Set multiple Set-Cookie headers #98

Closed
pilcrowOnPaper opened this issue Sep 4, 2023 · 4 comments
Closed

Feature request: Set multiple Set-Cookie headers #98

pilcrowOnPaper opened this issue Sep 4, 2023 · 4 comments

Comments

@pilcrowOnPaper
Copy link

pilcrowOnPaper commented Sep 4, 2023

Issue

Looking through the source code of @elysiajs/cookie, the only way to set multiple Set-Cookie headers currently is to pass any array to context.set.headers["Set-Cookie"]. An obvious issue is that you have to ignore TS errors, but another issue is that you can override existing Set-Cookie headers.

I'm aware I can just use @elysiajs/cookie, but not having a native way to handle it (without TS errors) is an issue IMO. Personally, I'm creating a library API that works with cookies and not having a solution natively sucks.

Possible solutions

Context.set.cookie()

This similar to res.cookie() in Express. I'm not sure if it's better to accept raw Set-Cookie header or use a similar syntax to @elysiajs/cookie where it takes a name, value, and attributes. Either way, it will create a new Set-Cookie header rather than overriding the existing value.

const serializedCookie = cookie.serialize(name, value, attributes);
context.set.cookie(serializedCookie);

Replace Context.set with Context.response

You still can keep the set name but I don't think set.setHeaders or set.appendHeaders() makes sense. Both of these proposal uses a similar API to native Headers.

context.response.headers.set(name, value);
context.response.headers.append(name, value);
context.response.setHeaders(name, value);
context.response.appendHeaders(name, value);

Also, it makes more sense to get response header values from context.response compared to context.set.

Update type of Context.set.headers to Record<string, string | string[]>

The same API and implementation but just updated types.

const responseSetCookieHeader = response.set.headers["Set-Cookie"];
context.set.headers["Set-Cookie"] = Array.isArray(responseSetCookieHeader) ? responseSetCookieHeader : [responseSetCookieHeader];
@SaltyAom
Copy link
Member

SaltyAom commented Sep 4, 2023

We are planning to merge cookie plugin into Elysia core to resolve the TypeScript error, but we are still deciding on the syntax of it.

In the meantime, I added a set.headers to accept Set-Cookie to be string | string[] to handle TypeScript error which is available in 0.6.17.

@pilcrowOnPaper
Copy link
Author

Sweet! Is there an issue for tracking that merge?

@SaltyAom
Copy link
Member

SaltyAom commented Sep 4, 2023

Likely going to be under miyu branch, a candidate for the next release.

Currently, I'm implementing tracing and adding unit tests, so it might take some time until the cookie is properly done, I'll let you know once the cookie is ready to be tested.

@SaltyAom SaltyAom mentioned this issue Sep 6, 2023
87 tasks
@SaltyAom
Copy link
Member

SaltyAom commented Sep 20, 2023

This should be fixed with Reactive Cookie in 0.7 #103

Or you can either use set.cookie like this:

set.cookie = {
    a: {
        value: 'a'
    },
    b: {
        value: 'b'
    }
}

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

No branches or pull requests

2 participants