add new cookies API #6593
add new cookies API #6593
Conversation
🦋 Changeset detectedLatest commit: cc6c56f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Looking good, but still wondering if we should allow cookies in setHeaders. If you set some simple cookies, you don't need to reach for that, and it may have this "oh why can't I use the platform" feeling to it. More comments inline, but nothing blocking this from merging.
| } | ||
| throw new Error( | ||
| `Use \`event.cookie.set(name, value, options)\` instead of \`event.setHeaders\` to set cookies` | ||
| ); |
There was a problem hiding this comment.
Instead of throwing here, couldn't we invoke something like cookies.set(cookie.parse(value)) here?
There was a problem hiding this comment.
We could, but no-one wants to write this...
import * as cookie from 'cookie';
export const actions = {
default: async ({ setHeaders }) => {
// ...
setHeaders({
'set-cookie': cookie.serialize('answer', 42, {
httpOnly: true
})
});
}
};...when they could write this:
export const actions = {
default: async ({ cookies }) => {
// ...
cookies.set('answer', 42);
}
};I for one would be grateful for an error message that said 'you should delete a bunch of code, and you can probably remove the cookie dependency'.
It also feels very weird if you use setHeaders followed by cookies.get — it doesn't feel like it should work, and yet it does. Always better to have one way to do things.
|
|
||
| for (const cookie of new_cookies) { | ||
| if (cookies.includes(cookie)) { | ||
| throw new Error(`"${key}" header already has cookie with same value`); |
There was a problem hiding this comment.
I believe this check is no longer done in the new version - any drawbacks omitting it and we should add it back, or is it fine like this?
There was a problem hiding this comment.
It's harmless (if pointless) to duplicate cookies, the later one wins. To recreate this check we'd have to serialize eagerly, but it would only catch this fairly weird edge case — would probably make more sense to prevent different cookies, but I didn't want to get into questions about what to do with domain or path don't match, so I figured I'd just rely on people not being daft
| }); | ||
| } | ||
| ``` | ||
| You cannot add a `set-cookie` header with `setHeaders` — use the [`cookies`](/docs/types#sveltejs-kit-cookies) API in a server-only `load` function instead. |
There was a problem hiding this comment.
This makes me wonder if we should have a section "inputs exclusive to isomorphic load" and "inputs exlusive to server-only load" in here (can be done in a separate PR)
There was a problem hiding this comment.
Probably yeah, since we haven't actually introduced RequestEvent by this stage
| cookies.push(cookie); | ||
| } | ||
| throw new Error( | ||
| `Use \`event.cookie.set(name, value, options)\` instead of \`event.setHeaders\` to set cookies` |
There was a problem hiding this comment.
Should't this be event.cookies.set?
closes #6540
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0