Skip to content

refactor(shims): dedupe cookie value serialization - #1072

Merged
james-elicx merged 2 commits into
mainfrom
refactor/dedupe-cookie-serialization
May 5, 2026
Merged

refactor(shims): dedupe cookie value serialization#1072
james-elicx merged 2 commits into
mainfrom
refactor/dedupe-cookie-serialization

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

Extract the shared Set-Cookie serialization from the next/headers and next/server shims into a single helper at packages/vinext/src/shims/internal/cookie-serialize.ts. Follow-up to #1049.

The two call sites — cookies().set() in headers.ts and ResponseCookies.set() in server.ts — were producing byte-for-byte identical Set-Cookie strings via copy-pasted code (same encodeURIComponent(value), same default Path=/, same attribute order: Path → Domain → Max-Age → Expires → HttpOnly → Secure → SameSite, same validators). Pure refactor — no behavior change.

The third candidate site, RequestCookies._serialize in server.ts:574, builds a request Cookie: header (name=value; name=value, no attributes) — different format, intentionally left alone.

What moved

packages/vinext/src/shims/internal/cookie-serialize.ts (new):

  • serializeSetCookie(name, value, options) — builds the full Set-Cookie value string
  • validateCookieName(name) — RFC 6265 §4.1.1 token validator
  • validateCookieAttributeValue(value, attr) — control-char/semicolon guard

The validators were also duplicated verbatim across both files; consolidating them avoids subtle drift.

Why a separate module instead of importing across shims

server.ts deliberately avoids static-importing headers.ts because of a Vite "use cache" transform issue (existing comment in server.ts:18-30). The new helper lives under internal/ alongside parse-cookie-header.ts, mirroring the existing pattern.

Files changed

  • packages/vinext/src/shims/internal/cookie-serialize.ts (new, 84 lines)
  • packages/vinext/src/shims/headers.ts (-43 lines, unchanged behavior)
  • packages/vinext/src/shims/server.ts (-37 lines, unchanged behavior)

Net: 9 insertions, 79 deletions across the two existing files.

Test plan

  • pnpm vp test run tests/app-router.test.ts — 308 tests pass
  • pnpm vp test run tests/pages-router.test.ts — 200 tests pass (one pre-existing afterAll timeout in the unrelated allowedDevOrigins suite, reproduces on main without these changes)
  • pnpm vp test run tests/shims.test.ts tests/app-route-handler-execution.test.ts tests/app-page-execution.test.ts tests/api-handler.test.ts — 912 tests pass
  • pnpm tsc --noEmit (in packages/vinext) — clean
  • pnpm fmt --write on touched files

🤖 Generated with Claude Code

Extract the shared encodeURIComponent-based value encoding from
headers.ts and server.ts ResponseCookies into a single helper.
Follow-up to #1049.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pkg-pr-new

pkg-pr-new Bot commented May 5, 2026

Copy link
Copy Markdown

Open in StackBlitz

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

commit: 2fd7dfe

@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original

knip flagged this as an unused export — the type is only consumed
within cookie-serialize.ts itself.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean refactor. The extracted helper is byte-for-byte identical to the two inlined copies it replaces, the internal/ placement follows the existing pattern (parse-cookie-header.ts), and the static-import constraint from server.ts → headers.ts (Vite "use cache" transform) is properly sidestepped.

Two minor observations — neither blocking:

  1. RequestCookies.delete in headers.ts still builds its deletion string inline (lines 907-923) rather than going through serializeSetCookie. The format is slightly different (bare name= vs name=%s via encodeURIComponent(""), and it omits HttpOnly/Secure/SameSite), so leaving it separate is reasonable. But worth noting that if the deletion format ever needs to change, there are now three serialization sites rather than two — this one, plus the ResponseCookies.delete path (which delegates to set()serializeSetCookie).

  2. SerializeSetCookieOptions vs CookieOptions — the new module defines its own structurally-identical type rather than importing from server.ts (which would reintroduce the circular-import concern). That's correct, but if CookieOptions later gains fields (e.g. partitioned, priority), both types need updating. A shared type under internal/ would prevent drift, though that's a separate cleanup.

LGTM — the dedup is well-scoped and the PR description clearly explains what moved and what was intentionally left alone.

@ask-bonk

ask-bonk Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

Review posted with approval on PR #1072.

Summary: This is a clean, well-scoped dedup refactor. The new cookie-serialize.ts helper under internal/ is byte-for-byte identical to the two inlined copies it replaces. The placement follows the existing internal/ directory pattern, and the approach correctly avoids the known server.ts → headers.ts static-import issue (Vite "use cache" transform).

I noted two non-blocking observations:

  1. RequestCookies.delete in headers.ts still builds its deletion string inline (slightly different format — no value encoding, no HttpOnly/Secure/SameSite), so there are technically three serialization sites now rather than one.
  2. SerializeSetCookieOptions and CookieOptions are structurally identical but separate types — moving CookieOptions to internal/ would prevent future drift if fields like partitioned are added.

github run

@james-elicx
james-elicx marked this pull request as ready for review May 5, 2026 18:24
@james-elicx
james-elicx merged commit 61a4551 into main May 5, 2026
33 checks passed
@james-elicx
james-elicx deleted the refactor/dedupe-cookie-serialization branch May 5, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant