Skip to content

Conversation

msonnb
Copy link
Contributor

@msonnb msonnb commented Mar 2, 2023

closes #9263

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.
Copy link
Member

@Rich-Harris Rich-Harris left a 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):

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:

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.

@msonnb
Copy link
Contributor Author

msonnb commented Mar 2, 2023

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):

sure, added the checks for domain and path 👍

Also, we need to consider existing cookies, not just new ones:
New cookies should take priority though, if cookies.set('foo', ...) was called and there was already a foo cookie in the request header.

should already work, no? at first we get all existing cookies with parse, then add all new ones and (potentially) override existing keys.

@Rich-Harris
Copy link
Member

Ah whoops, I missed that line at the beginning! Thank you

@Rich-Harris Rich-Harris merged commit 2b647fd into sveltejs:master Mar 2, 2023
@github-actions github-actions bot mentioned this pull request Mar 2, 2023
@msonnb msonnb deleted the cookies-get-all branch March 2, 2023 21:10
@david-plugge
Copy link
Contributor

Thanks alot!

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

Labels

None yet

3 participants