fix: align next/headers cookie path semantics#555
Conversation
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Review: align next/headers cookie path semantics
Good change overall — defaulting Path=/ on set() and accepting the object form on delete() are both real behavioral gaps. The implementation is clean, tests are well structured, and the integration test updates are correct.
A few items below, one of which is a bug.
| @@ -790,10 +789,22 @@ class RequestCookies { | |||
| /** | |||
| * Delete a cookie by setting it with Max-Age=0. | |||
There was a problem hiding this comment.
The JSDoc still says "Delete a cookie by setting it with Max-Age=0" but the implementation now uses Expires=Thu, 01 Jan 1970 00:00:00 GMT. Should be updated to match the new behavior.
| * Delete a cookie by setting it with Max-Age=0. | |
| * Delete a cookie by emitting an expired Set-Cookie header. |
There was a problem hiding this comment.
Updated the JSDoc to describe the actual deletion mechanism. The implementation emits an expired Set-Cookie header now, so the old Max-Age=0 wording was stale.
| _getState().pendingSetCookies.push(`${name}=; Path=/; Max-Age=0`); | ||
| const parts = [`${name}=`, `Path=${path}`]; | ||
| if (domain) parts.push(`Domain=${domain}`); | ||
| parts.push(`Expires=${new Date(0).toUTCString()}`); |
There was a problem hiding this comment.
This creates a new Date(0) object on every delete() call. Since the value is always the same (Thu, 01 Jan 1970 00:00:00 GMT), consider hoisting it to a module-level constant:
const EXPIRED_COOKIE_DATE = new Date(0).toUTCString();Minor optimization but it's a hot path in route handlers that clear multiple cookies.
There was a problem hiding this comment.
Hoisted the expired date string to a module constant. That keeps the hot path honest without changing the cookie semantics.
| * Delete a cookie by setting it with Max-Age=0. | ||
| */ | ||
| delete(name: string): this { | ||
| delete(nameOrOptions: string | { name: string; path?: string; domain?: string }): this { |
There was a problem hiding this comment.
Bug: delete() only accepts string | { name, path?, domain? } but Next.js also accepts an array of names — cookies().delete(["a", "b"]). The old delete(name: string) didn't support it either, so this isn't a regression, but since you're touching the signature it's worth noting.
More importantly: Next.js's RequestCookies.delete() in @edge-runtime/cookies (which is what cookies() from next/headers delegates to internally) uses the signature delete(names: string | string[]) — it accepts arrays. If you want to match upstream, consider adding array support too. That said, the object form { name, path, domain } you're adding here is correct for ResponseCookies.delete() semantics, which is the right reference since cookies().delete() in route handlers actually produces Set-Cookie headers (response-side behavior).
Not a blocker — the current scope is fine — but documenting this for awareness.
There was a problem hiding this comment.
Kept array support out of this PR. The bug here was path/default-path parity, and widening the delete signature further would expand the public API surface beyond the tracked issue.
| * Delete a cookie by setting it with Max-Age=0. | ||
| */ | ||
| delete(name: string): this { | ||
| delete(nameOrOptions: string | { name: string; path?: string; domain?: string }): this { |
There was a problem hiding this comment.
Nit: The PR description mentions this aligns with ResponseCookies.delete() behavior. However, looking at server.ts:464, NextResponse.cookies.delete() still uses Max-Age=0 (via this.set(name, "", { maxAge: 0, path: "/" })) rather than Expires=. This means the two cookie surfaces in vinext now use different deletion mechanisms.
Both are valid ways to expire a cookie per RFC 6265, but it's a divergence worth being aware of. Consider noting in a follow-up whether NextResponse.cookies.delete() should also switch to the Expires= pattern, or whether next/headers cookies().delete() should stay with Max-Age=0 for internal consistency.
There was a problem hiding this comment.
Left the NextResponse.cookies.delete() consistency question for follow-up. Both forms are valid expiry mechanisms, and this PR is scoped to the next/headers parity gap rather than normalizing both cookie surfaces at once.
| const pending = getAndClearPendingCookies(); | ||
| expect(pending).toHaveLength(1); | ||
| expect(pending[0]).toContain("token=implicit-path"); | ||
| expect(pending[0]).toContain("Path=/"); |
There was a problem hiding this comment.
Good test — this is the key regression test for the path default behavior. Consider also adding a case that verifies an explicit path option still works on set() (i.e., c.set("token", "val", { path: "/api" }) produces Path=/api, not Path=/). This ensures the default doesn't accidentally override an explicit path.
Actually, looking more carefully at the existing test on line ~895 (cookies().set() with options in the action phase), it does test { httpOnly: true } but not an explicit path. So there's currently no test ensuring explicit path is respected.
There was a problem hiding this comment.
Added an explicit-path regression test for cookies().set(). It now proves the default path logic only applies when the caller omits path.
| const previousPhase = setHeadersAccessPhase("route-handler"); | ||
| try { | ||
| const c = await cookies(); | ||
| c.delete({ name: "session", path: "/account", domain: ".example.com" }); |
There was a problem hiding this comment.
Nice — this covers the full object form well. One additional edge case worth testing: delete({ name: "session" }) without path or domain should still emit Path=/ (the default). This verifies the nameOrOptions.path ?? "/" fallback in the object branch.
There was a problem hiding this comment.
Added the object-form fallback case as well. delete({ name: "session" }) now has coverage for the implicit Path=/ branch.
|
Review posted successfully. Here's a summary of the feedback: Key findings:
|
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: align next/headers cookie path semantics
This is a clean, well-scoped fix. The two behavioral changes — defaulting Path=/ on set() and accepting the object form on delete() — are both real gaps against Next.js semantics. The implementation is correct, the test coverage is thorough (including the explicit-path and object-form-fallback cases added after the first review round), and the integration test updates are appropriate.
The previous review covered this well and the author addressed all actionable feedback (hoisted constant, updated JSDoc, added edge case tests). I have one minor observation below but nothing blocking.
LGTM.
|
|
||
| this._cookies.delete(name); | ||
| _getState().pendingSetCookies.push(`${name}=; Path=/; Max-Age=0`); | ||
| const parts = [`${name}=`, `Path=${path}`]; |
There was a problem hiding this comment.
Nit: the deleted cookie value is empty (session=) which is correct, but the formatting produces session=; Path=/; Expires=... — note the space after the empty value before the semicolon. This matches the set() method's parts.join("; ") pattern so it's consistent internally, but Next.js's ResponseCookies.delete() calls this.set(name, "", ...) which produces session=; Path=/; Max-Age=0 (also empty value). So both sides are consistent on the empty-value-then-attributes format. No issue here, just confirming I checked.
| * Delete a cookie by emitting an expired Set-Cookie header. | ||
| */ | ||
| delete(name: string): this { | ||
| delete(nameOrOptions: string | { name: string; path?: string; domain?: string }): this { |
There was a problem hiding this comment.
For the record: ResponseCookies.delete() in server.ts:464 still uses Max-Age=0 via this.set(name, "", { maxAge: 0, path: "/" }), while this method now uses Expires=Thu, 01 Jan 1970 00:00:00 GMT. Both are valid per RFC 6265, and the author explicitly noted this is a follow-up concern. Just flagging for visibility since these are the two cookie-deletion surfaces in vinext — if a future PR normalizes them, either direction would be fine.
The scope decision to keep this PR focused on next/headers parity is the right call.
|
Review submitted. Here's a summary of my assessment: Verdict: Approved The PR correctly fixes two gaps in
The code is clean, the I left two informational comments noting the |
Summary
This aligns the mutable
next/headerscookie shim with Next.js cookie semantics in two places:cookies().set()now defaultsPath=/, andcookies().delete()now accepts an options object withpathanddomain.What Changed
cookies().set()to emitPath=/when no path is providedcookies().delete({ name, path, domain })and emit an expired cookie header with those attributesMax-Age=0tests/shims.test.tsfor both behaviorsWhy It Matters
Without the default path,
cookies().set("name", "value")scopes the cookie to the current request path instead of/, which differs from Next.js and can break session visibility across routes. Without the object-form delete, vinext cannot reliably clear cookies that were originally set on a non-default path or domain.Risks Or Limits
This only changes the mutable
next/headerscookie surface inshims/headers.ts. It does not changeNextResponse.cookiesinnext/server. The deletion header now uses an expiredExpires=value, matching upstreamResponseCookies.delete()behavior.Verification
./node_modules/.bin/vp test run tests/shims.test.ts -t "next/headers phase-aware cookies"./node_modules/.bin/vp test run tests/nextjs-compat/app-routes.test.ts tests/nextjs-compat/set-cookies.test.ts tests/app-router.test.tsgit diff --check