feat(cookies): align RequestCookies and ResponseCookies
#181
Conversation
🦋 Changeset detectedLatest commit: e1d71d0 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 ↗︎
|
balazsorban44
changed the title
RequestCookies and ResponseCookiesRequestCookies and ResponseCookies
Codecov Report
@@ Coverage Diff @@
## main #181 +/- ##
==========================================
- Coverage 87.91% 84.53% -3.38%
==========================================
Files 14 21 +7
Lines 1704 1785 +81
Branches 213 182 -31
==========================================
+ Hits 1498 1509 +11
- Misses 192 252 +60
- Partials 14 24 +10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| constructor(request: Request) { | ||
| this.#headers = request.headers |
There was a problem hiding this comment.
I used that type to hint that we need a request here and not a response
There was a problem hiding this comment.
new RequestCookies(request) looks nicer than new RequestCookies(request.headers), the latter feels kinda leaky to me. wdyt?
| clear(...args: [key: string] | [options: ResponseCookie] | []): this { | ||
| const key = typeof args[0] === 'string' ? args[0] : args[0]?.name | ||
| this.getAll(key).forEach((c) => this.delete(c)) | ||
| if (key) this.getAll(key).forEach((c) => this.delete(c)) | ||
| return this | ||
| } |
There was a problem hiding this comment.
I wonder if this method is confusing. response.cookies.clear() might make people believe they can clear all user cookies. this is not the case. Although, this clearly say "response.cookies.clear" so maybe it's not confusing?
There was a problem hiding this comment.
maybe we should iterate this in the comment?
There was a problem hiding this comment.
Made it so that you can call response.cookies.clear() which will on invalidate all cookies. 👍 added tests for it too
| constructor(responseHeaders: Headers) { | ||
| this.#headers = responseHeaders |
Co-authored-by: Gal Schlezinger <gal@spitfire.co.il>
…b.com:balazsorban44/edge-runtime into feat/align-request-response-cookie-interfaces
…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)
Co-authored-by: Gal Schlezinger <gal@spitfire.co.il>
Using the same interface as CookieStore whenever possible.
Also, both
RequestCookiesandResponseCooieswill only receiveHeadarsin the constructor, as they don't need more than that. (strictly speaking, we could just pass the cookie header value too...)