elysiajs / elysia

Ergonomic Framework for Humans
https://elysiajs.com
MIT License
10.25k stars 219 forks source link

Feature request: Set multiple `Set-Cookie` headers #98

Closed pilcrowonpaper closed 1 year ago

pilcrowonpaper commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Sweet! Is there an issue for tracking that merge?

SaltyAom commented 1 year ago

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 commented 1 year ago

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'
    }
}