Add parse method for set-cookie#244
Conversation
| _name: string | SetCookie, | ||
| _val?: string | SerializeOptions, | ||
| _opts?: SerializeOptions & Omit<SetCookie, "name" | "value">, | ||
| ): string { |
There was a problem hiding this comment.
Alternatively, could avoid changing this interface, but it's nice to have parity with serialize(deserialize("")).
There was a problem hiding this comment.
High level comment here, but I feel like having these "generic" names like stringify and serialize don't do a great job of showing users what they are intended to be used for. Part of the ongoing confusion in this package has been that folks thought stringify could be used with set-cookie. Now I am concerned that the new name will mean folks are confused on which is for the cookie and which is for set-cookie.
I think there are two ways I could see us improving this:
- A naming convention like
encodeSetCookie/encodeCookie. Downside of longer names and having to alias & deprecateencode(maybe more?). Upside, has a clean path to landing here without major disruption and clarifies the intent. - Namespacing. There are a few ways I can think of (
cookie.set.encode/cookie.get.encode,cookie.encode.set/cookie.encode.get, names to be bikeshed), but I think I am more in favor of option one, so don't want to spend too much time on this one unless other folks really like this idea.
I read through some of the details, but other than small nitpicks I might make I don't have a lot to add. Leaving as a comment so I don't accidentally block, but I am pretty concerned with the point I raised above.
|
For how my brain works, the API here is hard to hold onto. I can't keep straight what methods are meant for what header type After reviewing the code, and not looking at it right now, let me try to recall.
That's my biggest nit here. What about exploring aliasing to prevent needing a major ala: export {
parse,
stringify,
parse as parseCookie,
stringify stringifyCookie,
serialize,
deserialize,
serialize as serializeSetCookie,
deserialize as deserializeSetCookie
};Or otherwise namespacing to make it clearer I think standardizing on a single verb like parse/serialize and tacking on the header type would make it a more straightforward DX as well. is there a real difference between parse/deserialize besides the type of header they operate on and the algorithm they use? |
|
Agree with the naming, I just stuck to the existing scheme. The existing methods were I can rename in a follow up PR or this one, whichever is preferred. Though I'd go with
I'm not sure how to answer this, but no, aside from the entire algorithm they are meant to be mirrors of each other. Ideally you can do |
| ## Encode and decode | ||
|
|
||
| Cookie accepts `encode` and `decode` options to serialize and deserialize a [cookie-value](https://datatracker.ietf.org/doc/html/rfc6265#section-4.1.1). | ||
| Since the value of a cookie has a limited character set (and must be a simple string), these functions are be used to transform values into strings suitable for a cookies value. |
There was a problem hiding this comment.
| Since the value of a cookie has a limited character set (and must be a simple string), these functions are be used to transform values into strings suitable for a cookies value. | |
| Since the value of a cookie has a limited character set (and must be a simple string), these functions are used to transform values into strings suitable for a cookies value. |
was just a politer way to say "it doesnt make any sense to me that they're not the same name" I know it's not a choice you made hehe |
Haha cool, it's fine to say that, it's reasonable. Do you think they should both be verbose? E.g. I'm doing |
jonchurch
left a comment
jonchurch
left a comment
There was a problem hiding this comment.
Im down with the change.
I do want to see the naming of the methods cleaned up, but this PR is 👍 as is
| /** | ||
| * Backward compatibility exports. | ||
| */ | ||
| export { stringifySetCookie as serialize, parseCookie as parse }; |
There was a problem hiding this comment.
@jonchurch Methods have been renamed, here's the backward compatibility exports (no version with stringify was released).
blakeembrey
changed the title
set-cookieset-cookie
|
The spec itself has us ignoring invalid fields, e.g.
That said, we're not specifically following the spec in other places. For examples, it's more lenient on no validation for domain or path, or it removes prefixed I'm leaning most toward following the spec, e.g. ignoring invalid I do like the idea of an escape hatch allow users to potentially parse a raw array + serialize that without all the validation, though that's an entirely different implementation. |
Drops the `set-cookie-parser` dependency (and its `@types/*` package) by
using `parseSetCookie` from the already-bundled `cookie@1.x` package and
the standard `Headers.prototype.getSetCookie()` API (Node.js ≥ 18).
### Key changes
- **`server/fetch.js`**: Replace `splitCookiesString` + `parseString`
with `response.headers.getSetCookie()` (returns individual Set-Cookie
headers as an array) + `parseSetCookie(str, { decode: (v) => v })`
- **`exports/node/index.js`**: Replace
`splitCookiesString(response.headers.get('set-cookie'))` workaround with
`response.headers.getSetCookie()` directly
- **`exports/vite/index.js`**: Remove `set-cookie-parser` from SSR
externals list
- **`package.json` / `pnpm-workspace.yaml`**: Remove `set-cookie-parser`
and `@types/set-cookie-parser`
```js
// Before
for (const str of set_cookie_parser.splitCookiesString(response.headers.get('set-cookie'))) {
const { name, value, ...options } = set_cookie_parser.parseString(str, { decodeValues: false });
// ...
}
// After
for (const str of response.headers.getSetCookie()) {
const { name, value, ...options } = parseSetCookie(str, { decode: (v) => v });
// ...
}
```
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
>
> ----
>
> *This section details on the original issue you should resolve*
>
> <issue_title>Various cookie upgrades and package
removals</issue_title>
> <issue_description>### Describe the problem
>
> Upgrade to cookie 1.x
> Allows dropping types package
> Next release will allow dropping set-cookie-parser
(jshttp/cookie#244)
>
> ### Describe the proposed solution
>
> Update to latest cookie package
>
> ### Alternatives considered
>
> _No response_
>
> ### Importance
>
> nice to have
>
> ### Additional Information
>
> _No response_</issue_description>
>
> <agent_instructions>use `parseSetCookie` from the `cookie` package to
replace the `set-cookie-parser` package</agent_instructions>
>
> ## Comments on the Issue (you are @copilot in this section)
>
> <comments>
> <comment_new><author>@Conduitry</author><body>
> We don't want to work on this until we're ready to start making the
push to SvelteKit 3.0 as it's going to be a breaking
change.</body></comment_new>
> <comment_new><author>@benmccann</author><body>
> We tried to upgrade cookie earlier, but had to roll it back due to
breaking changes. SvelteKit 3 will happen soon and will upgrade to the
latest </body></comment_new>
> <comment_new><author>@benmccann</author><body>
> @copilot use `parseSetCookie` from the `cookie` package to replace the
`set-cookie-parser` package in the `version-3`
branch</body></comment_new>
> </comments>
>
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
- Fixes #14553
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/sveltejs/kit/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: benmccann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Tee Ming <chewteeming01@gmail.com>
4 participants
Other part of #200, follow up to #214. Adds a deserialize method that mirrors the
serializemethod.The current signature of serialize is
(key, val, options)which doesn't lend itself to being mirrored particularly nicely. In this PR I've opted to support an objectsetCookiewithkeyandvalueproperties, and kept the old signature for backward compatibility. Alternatively, I could return a tuple, butoptionsisn't the cleanest already with both values that get used forset-cookieandencodewhich is a function used forvalue.