feat(cookies): make RequestCookies and ResponseCookies more spec compliant
#177
Conversation
🦋 Changeset detectedLatest commit: b471e97 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
@@ Coverage Diff @@
## main #177 +/- ##
==========================================
- Coverage 87.91% 84.96% -2.95%
==========================================
Files 14 21 +7
Lines 1704 1803 +99
Branches 213 181 -32
==========================================
+ Hits 1498 1532 +34
- Misses 192 251 +59
- Partials 14 20 +6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| * Uses {@link ResponseCookies.get} to return only the cookie value. | ||
| */ | ||
| getValue(...args: [key: string] | [options: Cookie]): string | undefined { | ||
| return this.get(...args)?.value |
There was a problem hiding this comment.
Arguably, this method should be unnecessary:
.getValue("cookie") is the same as
.get("cookie")?.value
Breaking spec to save 2 characters might not be worth it.
| @@ -1,3 +1,3 @@ | |||
| export type { Options } from './serialize' | |||
| export type { Cookie, CookieListItem } from './serialize' | |||
There was a problem hiding this comment.
CookieOptions looks more natural to me, although I can see this is a thing defined by the spec interface
There was a problem hiding this comment.
It's essentially the entire parsed cookie, with name and value. Only wording, so I am OK with changing it, but in my mind Cookie made more sense.
| } | ||
| getAll(...args: [name: string] | [Cookie] | [undefined]) { | ||
| const all = Array.from(this.#parsed()) | ||
| if (!args.length) { |
There was a problem hiding this comment.
Maybe a strict comparison could be desirable
args.length === 0
There was a problem hiding this comment.
Because of TypeScript, this would show:
This condition will always return 'false' since the types '1' and '0' have no overlap.ts(2367)
Since we technically don't allow more than one argument, but the user might/will try. !args.length should be as safe, since args is always going to be an array.
| import { cached } from './cached' | ||
|
|
||
| /** | ||
| * A class for manipulating {@link Request} cookies. | ||
| */ | ||
| export class RequestCookies { | ||
| private readonly headers: Headers | ||
| readonly #headers: Headers |
There was a problem hiding this comment.
the future is here and now
…and `ResponseCookies` (#41526) Ref: [Slack thread](https://vercel.slack.com/archives/C035J346QQL/p1666056382299069?thread_ts=1666041444.633059&cid=C035J346QQL), [docs update](vercel/front#17090) Spec: https://wicg.github.io/cookie-store/ BREAKING CHANGE: Ref: vercel/edge-runtime#177, vercel/edge-runtime#181 ## Bug - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Errors have a helpful link attached, see `contributing.md` ## Feature - [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [ ] Errors have a helpful link attached, see `contributing.md` ## Documentation / Examples - [ ] Make sure the linting passes by running `pnpm lint` - [ ] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md)
…compliant (vercel#177) * feat(cookies): make `RequestCookies` and `ResponseCookies` more spec compliant * add changeset * mention breaking changes * test `get()?.value` * address review
Changes based on the conversation in this Slack thread.
Spec: https://wicg.github.io/cookie-store
Notable breaking changes:
ResponseCookies#gethas been renamed toResponseCookies#getValueResponseCookies#getWithOptionshas been renamed toResponseCookies#get