-
-
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
feat: add cookies.getAll
#9287
feat: add cookies.getAll
#9287
Conversation
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.
Thank you! Couple of changes I think we need — this logic needs to be reflected (i.e. we only want to include cookies with a valid path
etc — if cookies.get('foo')
wouldn't return a value, then it shouldn't be included in getAll
):
kit/packages/kit/src/runtime/server/cookie.js
Lines 75 to 76 in 90fdfdb
domain_matches(url.hostname, c.options.domain) && | |
path_matches(url.pathname, c.options.path) |
Also, we need to consider existing cookies, not just new ones:
kit/packages/kit/src/runtime/server/cookie.js
Lines 82 to 83 in 90fdfdb
const req_cookies = parse(header, { decode: decoder }); | |
const cookie = req_cookies[name]; // the decoded string or undefined |
New cookies should take priority though, if cookies.set('foo', ...)
was called and there was already a foo
cookie in the request header.
sure, added the checks for domain and path 👍
should already work, no? at first we get all existing cookies with |
Ah whoops, I missed that line at the beginning! Thank you |
Thanks alot! |
closes #9263
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.