fix: add missing set/delete/clear/size/toString to RequestCookies by NathanDrake2406 · Pull Request #517 · cloudflare/vinext

NathanDrake2406 · 2026-03-13T05:55:36Z

Summary

The RequestCookies class in server.ts (used by NextRequest.cookies in middleware) was missing set(), delete(), clear(), size, and toString() methods that @edge-runtime/cookies provides
Middleware calling cookies.set() or cookies.delete() to forward modified cookies downstream would fail at runtime
Caches the parsed cookie map in the constructor instead of re-parsing the Cookie header on every method call
Encodes values with encodeURIComponent when syncing back to the Cookie header, preventing round-trip data corruption
delete() accepts string | string[] and returns boolean | boolean[] matching @edge-runtime/cookies

Test plan

16 new tests in RequestCookies API describe block covering:
- set() — add, overwrite, object form, chaining
- delete() — single name, array of names, return value (boolean/boolean[]), no-op for missing
- clear() — removes all cookies and clears header
- size — count, empty
- toString() — serialization
- Round-trip encoding with special characters (;, =, %)
- Percent-encoded value preservation across mutations
- set() then delete() interaction
All 680 shims tests pass
Pre-commit: format, lint, typecheck all green

The RequestCookies class in server.ts (used by NextRequest.cookies) was missing mutation methods that @edge-runtime/cookies provides. Middleware that calls cookies.set() or cookies.delete() to forward modified cookies downstream would fail at runtime. - Add set(), delete(), clear(), size, toString() methods - Cache parsed map in constructor instead of re-parsing on every call - Encode values with encodeURIComponent when syncing back to Cookie header - delete() accepts string | string[] and returns boolean | boolean[] matching @edge-runtime/cookies API

pkg-pr-new · 2026-03-13T05:56:15Z

Open in StackBlitz

npm i https://pkg.pr.new/vinext@517

commit: eb7535c

james-elicx · 2026-03-13T06:10:37Z

/bigbonk review

ask-bonk

Good contribution — the missing set/delete/clear/size/toString methods are genuinely needed for middleware cookie forwarding, and the test coverage is solid. A few issues to address, mostly around behavioral divergence from @edge-runtime/cookies:

Behavioral divergence: delete() after deleting all cookies leaves a stale empty header

Upstream @edge-runtime/cookies always calls this._headers.set('cookie', ...) after delete — it never conditionally calls this._headers.delete('cookie'). Your _syncHeader deletes the header when the map is empty, which is fine (arguably better), but the delete() method has a subtle issue: when the array overload deletes the last cookie, it calls _syncHeader() once, which is correct. But this also means clear() and _syncHeader() have two different code paths for removing the header — clear() inlines this._headers.delete('cookie') while _syncHeader() also does it. Consider having clear() delegate to _syncHeader() for consistency.

toString() duplicates _syncHeader() logic

The serialization logic ([...entries].map(…).join(…)) is duplicated between toString() and _syncHeader(). This is a minor DRY issue but worth noting — if the encoding strategy ever changes, both need updating.

Overall this is in good shape. The main actionable item is the clear() consistency point (which is optional but would be cleaner). Tests are thorough.

ask-bonk · 2026-03-13T06:12:08Z

+  }
+
+  toString(): string {
+    return [...this._parsed.entries()].map(([n, v]) => `${n}=${encodeURIComponent(v)}`).join("; ");


toString() and _syncHeader() share the same serialization logic. Consider extracting a private helper to avoid duplication:

Suggested change

return [...this._parsed.entries()].map(([n, v]) => `${n}=${encodeURIComponent(v)}`).join("; ");

toString(): string {

return this._serialize();

}

Then add a private _serialize() method and call it from both toString() and _syncHeader(). This way if the encoding strategy changes (e.g., you switch to a stringifyCookie-style function), there's only one place to update.

ask-bonk · 2026-03-13T06:12:08Z

+  clear(): this {
+    this._parsed.clear();
+    this._headers.delete("cookie");
+    return this;


Nit: clear() manually inlines the header deletion (this._headers.delete("cookie")), while _syncHeader() also handles the empty-map case. For consistency, clear() could delegate to _syncHeader() instead:

Suggested change

return this;

clear(): this {

this._parsed.clear();

this._syncHeader();

return this;

}

This is functionally equivalent but avoids two separate code paths for removing the cookie header when the map is empty.

ask-bonk · 2026-03-13T06:12:08Z

+    return this;
+  }
+
+  delete(names: string | string[]): boolean | boolean[] {


Worth noting: @edge-runtime/cookies returns boolean | boolean[] from delete() with the same semantics (single → boolean, array → boolean[]). The return type matches upstream, which is good.

However, upstream's delete() always calls _headers.set('cookie', ...) even when the map is empty (resulting in an empty cookie: header), while this implementation calls _headers.delete('cookie') via _syncHeader(). The vinext behavior (removing the header entirely) is arguably more correct, just flagging the difference for awareness.

ask-bonk · 2026-03-13T06:12:23Z

Review posted. Here's a summary of my findings:

Overall: Good PR — fills a real gap in the RequestCookies API. The implementation is correct and the 16 new tests are thorough.

Key observations from comparing against @edge-runtime/cookies upstream:

clear() / _syncHeader() consistency — clear() manually inlines this._headers.delete("cookie"), while _syncHeader() also handles the empty-map case. Suggested having clear() delegate to _syncHeader() to avoid two code paths for the same operation.
toString() / _syncHeader() duplication — The serialization logic ([...entries].map(([n, v]) => ...).join("; ")) is duplicated between both methods. Suggested extracting a shared private helper.
Behavioral difference from upstream on empty map — Upstream always calls _headers.set('cookie', ...) after delete (which can leave an empty cookie: header), while this implementation deletes the header entirely when the map is empty. The vinext behavior is arguably more correct, just flagged for awareness.

The caching of the parsed map in the constructor (replacing the _parse() on every call pattern) is a clear improvement. The encodeURIComponent encoding on sync-back is correct and matches upstream's stringifyCookie. Return types for delete() match upstream (boolean | boolean[]).

github run

Address review feedback: - clear() delegates to _syncHeader() instead of inlining header deletion - Extract _serialize() helper shared by toString() and _syncHeader()

james-elicx · 2026-03-13T07:26:10Z

/bigbonk re-review

ask-bonk

Good contribution. The implementation matches @edge-runtime/cookies semantics where it matters, the code is clean, and the tests are thorough. I verified against the upstream source — the API surface, encoding strategy, and return types all align.

The previous review's suggestions (have clear() delegate to _syncHeader(), extract _serialize() helper) have already been addressed in this revision, which is nice to see.

One minor nit and one observation below, neither blocking.

ask-bonk · 2026-03-13T07:28:37Z

+    let cookieValue: string;
+    if (typeof nameOrOptions === "string") {
+      cookieName = nameOrOptions;
+      cookieValue = value ?? "";


Nit: when nameOrOptions is a CookieEntry object, the value parameter is ignored. Upstream does the same, but it might be worth a brief doc comment on the signature noting that value is only used when nameOrOptions is a string. Not blocking.

ask-bonk · 2026-03-13T07:28:37Z

+    expect(headers.get("cookie")).toBe("data=hello%3Bworld%3Dfoo");
+  });
+
+  it("set() preserves existing percent-encoded cookies", async () => {


This test checks that the get() value is correct after mutation, but doesn't assert on the serialized header. It might be worth adding an assertion like:

expect(headers.get("cookie")).toContain("token=100%2525done");

to verify the header round-trips correctly (the % in 100%done should be re-encoded as %25 by encodeURIComponent). Currently the test only verifies the in-memory parsed value, not the wire format.

Not blocking — the existing coverage is solid.

ask-bonk · 2026-03-13T07:28:46Z

Review submitted. Here's a summary of my findings:

Verdict: Approved

The PR correctly fills a real gap in the RequestCookies API. I verified the implementation against the upstream @edge-runtime/cookies source and confirmed:

API parity: set(), delete(), clear(), size, and toString() all match upstream signatures and semantics
Encoding strategy: Uses encodeURIComponent for serialization, matching upstream's stringifyCookie
Return types: delete() returns boolean | boolean[] matching upstream; set() and clear() return this for chaining
_serialize() helper: Already extracted as a shared method between toString() and _syncHeader() (addressing the earlier review feedback)
clear() delegation: Already delegates to _syncHeader() (also addressing earlier review feedback)

Minor intentional divergence (not a problem): When all cookies are deleted, this implementation removes the cookie header entirely (_headers.delete), while upstream sets it to an empty string (_headers.set('cookie', '')). The vinext behavior is cleaner.

Two non-blocking comments left:

A nit about documenting that the value parameter is ignored when nameOrOptions is an object
A suggestion to add a header-level assertion in the percent-encoding preservation test to verify the wire format, not just the in-memory value

github run

ask-bonk Bot reviewed Mar 13, 2026

View reviewed changes

refactor: DRY up RequestCookies clear/toString/syncHeader

d1dae48

Address review feedback: - clear() delegates to _syncHeader() instead of inlining header deletion - Extract _serialize() helper shared by toString() and _syncHeader()

ask-bonk Bot approved these changes Mar 13, 2026

View reviewed changes

james-elicx merged commit eed47a3 into cloudflare:main Mar 13, 2026
19 checks passed

NathanDrake2406 deleted the fix/request-cookies-set-delete branch March 18, 2026 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add missing set/delete/clear/size/toString to RequestCookies#517

fix: add missing set/delete/clear/size/toString to RequestCookies#517
james-elicx merged 2 commits intocloudflare:mainfrom
NathanDrake2406:fix/request-cookies-set-delete

NathanDrake2406 commented Mar 13, 2026

Uh oh!

pkg-pr-new Bot commented Mar 13, 2026 •

edited

Loading

Uh oh!

james-elicx commented Mar 13, 2026

Uh oh!

ask-bonk Bot left a comment

Uh oh!

ask-bonk Bot Mar 13, 2026

Uh oh!

ask-bonk Bot Mar 13, 2026

Uh oh!

ask-bonk Bot Mar 13, 2026

Uh oh!

ask-bonk Bot commented Mar 13, 2026

Uh oh!

james-elicx commented Mar 13, 2026

Uh oh!

ask-bonk Bot left a comment

Uh oh!

ask-bonk Bot Mar 13, 2026

Uh oh!

ask-bonk Bot Mar 13, 2026

Uh oh!

ask-bonk Bot commented Mar 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

NathanDrake2406 commented Mar 13, 2026

Summary

Test plan

Uh oh!

pkg-pr-new Bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-elicx commented Mar 13, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 13, 2026

Uh oh!

james-elicx commented Mar 13, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pkg-pr-new Bot commented Mar 13, 2026 •

edited

Loading