-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
add new cookies API #6593
add new cookies API #6593
Changes from 5 commits
f760a19
f28267b
ca248bc
3421c0d
fe96467
a93484c
cd7f2ef
cc6c56f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@sveltejs/kit': patch | ||
--- | ||
|
||
[breaking] add API for interacting with cookies |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
import * as cookie from 'cookie'; | ||
|
||
/** | ||
* @param {Request} request | ||
* @param {URL} url | ||
*/ | ||
export function get_cookies(request, url) { | ||
const initial_cookies = cookie.parse(request.headers.get('cookie') ?? ''); | ||
|
||
/** @type {Array<{ name: string, value: string, options: import('cookie').CookieSerializeOptions }>} */ | ||
const new_cookies = []; | ||
|
||
/** @type {import('types').Cookies} */ | ||
const cookies = { | ||
get(name, opts) { | ||
const decode = opts?.decode || decodeURIComponent; | ||
|
||
let i = new_cookies.length; | ||
while (i--) { | ||
const cookie = new_cookies[i]; | ||
|
||
if ( | ||
cookie.name === name && | ||
domain_matches(url.hostname, cookie.options.domain) && | ||
path_matches(url.pathname, cookie.options.path) | ||
) { | ||
return cookie.value; | ||
} | ||
} | ||
|
||
return decode(initial_cookies[name]); | ||
Rich-Harris marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
set(name, value, options = {}) { | ||
new_cookies.push({ | ||
name, | ||
value, | ||
options: { | ||
httpOnly: true, | ||
secure: true, | ||
...options | ||
} | ||
}); | ||
}, | ||
delete(name) { | ||
new_cookies.push({ name, value: '', options: { expires: new Date(0) } }); | ||
} | ||
}; | ||
|
||
return { cookies, new_cookies }; | ||
} | ||
|
||
/** | ||
* @param {string} hostname | ||
* @param {string} [constraint] | ||
*/ | ||
export function domain_matches(hostname, constraint) { | ||
if (!constraint) return true; | ||
|
||
const normalized = constraint[0] === '.' ? constraint.slice(1) : constraint; | ||
|
||
if (hostname === normalized) return true; | ||
return hostname.endsWith('.' + normalized); | ||
} | ||
|
||
/** | ||
* @param {string} path | ||
* @param {string} [constraint] | ||
*/ | ||
export function path_matches(path, constraint) { | ||
if (!constraint) return true; | ||
|
||
const normalized = constraint.endsWith('/') ? constraint.slice(0, -1) : constraint; | ||
|
||
if (path === normalized) return true; | ||
return path.startsWith(normalized + '/'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import * as cookie from 'cookie'; | ||
import { render_endpoint } from './endpoint.js'; | ||
import { render_page } from './page/index.js'; | ||
import { render_response } from './page/render.js'; | ||
|
@@ -8,6 +9,7 @@ import { decode_params, disable_search, normalize_path } from '../../utils/url.j | |
import { exec } from '../../utils/routing.js'; | ||
import { render_data } from './data/index.js'; | ||
import { DATA_SUFFIX } from '../../constants.js'; | ||
import { get_cookies } from './cookie.js'; | ||
|
||
/* global __SVELTEKIT_ADAPTER_NAME__ */ | ||
|
||
|
@@ -116,16 +118,16 @@ export async function respond(request, options, state) { | |
} | ||
} | ||
|
||
/** @type {import('types').ResponseHeaders} */ | ||
/** @type {Record<string, string>} */ | ||
const headers = {}; | ||
|
||
/** @type {string[]} */ | ||
const cookies = []; | ||
const { cookies, new_cookies } = get_cookies(request, url); | ||
|
||
if (state.prerendering) disable_search(url); | ||
|
||
/** @type {import('types').RequestEvent} */ | ||
const event = { | ||
cookies, | ||
getClientAddress: | ||
state.getClientAddress || | ||
(() => { | ||
|
@@ -144,15 +146,9 @@ export async function respond(request, options, state) { | |
const value = new_headers[key]; | ||
|
||
if (lower === 'set-cookie') { | ||
const new_cookies = /** @type {string[]} */ (Array.isArray(value) ? value : [value]); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep. fixed |
||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of throwing here, couldn't we invoke something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 It also feels very weird if you use |
||
} else if (lower in headers) { | ||
throw new Error(`"${key}" header is already set`); | ||
} else { | ||
|
@@ -275,8 +271,11 @@ export async function respond(request, options, state) { | |
} | ||
} | ||
|
||
for (const cookie of cookies) { | ||
response.headers.append('set-cookie', cookie); | ||
for (const new_cookie of new_cookies) { | ||
response.headers.append( | ||
'set-cookie', | ||
cookie.serialize(new_cookie.name, new_cookie.value, new_cookie.options) | ||
); | ||
} | ||
|
||
// respond with 304 if etag matches | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
export function load({ setHeaders }) { | ||
setHeaders({ | ||
'set-cookie': 'cookie1=value1' | ||
}); | ||
/** @type {import('./$types').LayoutServerLoad} */ | ||
export function load({ cookies }) { | ||
cookies.set('cookie1', 'value1'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
export function load({ setHeaders }) { | ||
setHeaders({ | ||
'set-cookie': 'cookie2=value2' | ||
}); | ||
/** @type {import('./$types').PageServerLoad} */ | ||
export function load({ cookies }) { | ||
cookies.set('cookie2', 'value2'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import { redirect } from '@sveltejs/kit'; | ||
|
||
export function load({ setHeaders }) { | ||
setHeaders({ 'set-cookie': 'shadow-redirect=happy' }); | ||
/** @type {import('./$types').PageServerLoad} */ | ||
export function load({ cookies }) { | ||
cookies.set('shadow-redirect', 'happy'); | ||
throw redirect(302, '/shadowed/redirected'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import { redirect } from '@sveltejs/kit'; | ||
|
||
export function POST({ setHeaders }) { | ||
setHeaders({ 'set-cookie': 'shadow-redirect=happy' }); | ||
/** @type {import('./$types').PageServerLoad} */ | ||
export function POST({ cookies }) { | ||
cookies.set('shadow-redirect', 'happy'); | ||
throw redirect(302, '/shadowed/redirected'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yeah, since we haven't actually introduced
RequestEvent
by this stage