feat(cookies): align RequestCookies and ResponseCookies#181
Merged
Schniz merged 10 commits intoOct 23, 2022
Merged
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 |
Contributor
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
RequestCookies and ResponseCookiesRequestCookies and ResponseCookies
This was referenced Oct 23, 2022
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 |
Schniz
reviewed
Oct 23, 2022
Comment on lines
-10
to
-11
| constructor(request: Request) { | ||
| this.#headers = request.headers |
Collaborator
There was a problem hiding this comment.
I used that type to hint that we need a request here and not a response
Collaborator
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?
Comment on lines
92
to
96
| 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 | ||
| } |
Collaborator
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?
Collaborator
There was a problem hiding this comment.
maybe we should iterate this in the comment?
Contributor
Author
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
Comment on lines
+13
to
+14
| 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
Schniz
approved these changes
Oct 23, 2022
ijjk
pushed a commit
to vercel/next.js
that referenced
this pull request
Oct 27, 2022
…and `ResponseCookies` (#41526) Ref: [Slack thread](https://vercel.slack.com/archives/C035J346QQL/p1666056382299069?thread_ts=1666041444.633059&cid=C035J346QQL), [docs update](https://github.com/vercel/front/pull/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)
jridgewell
pushed a commit
to jridgewell/edge-runtime
that referenced
this pull request
Jun 23, 2023
Co-authored-by: Gal Schlezinger <gal@spitfire.co.il>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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...)