Repository navigation
fix(pages): expose parsed cookies on request objects - #2401
Conversation
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): expose parsed cookies on request objects
I reviewed the diff, traced dev/prod/Workers parity, checked Next.js parity for the lazy-getter pattern, and ran the targeted tests. This is a solid, well-scoped fix. No blocking issues.
What's good
- Real bug fix, not cosmetic. Dev Pages SSR (
createSSRHandler) never setreq.cookiesat all before this —getServerSidePropssawundefined. Now both dev SSR and dev API paths get cookies, aligning with the Workers/prod adapter. - Correct dev/prod/Workers parity (the thing AGENTS.md warns about most):
- Prod + Workers Pages SSR/API go through
createPagesReqRes, which now callsattachPagesRequestCookiesinternally. - Dev + Node API go through
enhanceApiObjects(shared by dev and prod API dispatch) and the two dev-server SSR entry points (createSSRHandler,renderErrorPage). - All four request-construction sites are covered by a single shared helper — no divergent copies.
- Prod + Workers Pages SSR/API go through
- Faithful Next.js parity. Next.js uses
setLazyProp(req, 'cookies', getCookieParser(...))— an enumerable, configurable, self-replacing lazy getter that bails if the prop already exists.attachPagesRequestCookiesreplicates this exactly: lazy getter,Object.hasOwnguard, self-replacement on first read, and asettrap that keeps the prop writable. Good attention to detail. - Tests are well-chosen and ported with links back to the Next.js source/tests (
render.tsx,api-resolver.ts). They cover the empty-cookie ({}) case and the populated case, in both dev and the production end-to-end build.
Verification I ran
vp test run tests/pages-router.test.ts -t "passes parsed request cookies"→ 2 passed (SSR + API).vp test run tests/pages-router.test.ts -t "serves pages from production build end-to-end"→ passed, including the new prod SSR + API cookie assertions.vp checkon all changed source + fixtures → format/lint/type clean.
Minor observations (non-blocking, no change required)
string[]cookie-header branch is defensively dead.parsePagesRequestCookieshandlescookie?: string[]by joining with"; ", but neither carrier ever produces an array: NodeIncomingMessage.headers.cookiecollapses to a singlestring, andcreatePagesReqRes'sheadersObjisRecord<string,string>. Harmless and future-proof; just noting it's currently unreachable.renderErrorPagere-attach is redundant but correct. When the error page renders after a normal SSR pass,req.cookiesis already an own property, so theObject.hasOwnguard makes the secondattachPagesRequestCookies(req)a no-op. The explicit call is still worthwhile for error paths that bypasscreateSSRHandler(e.g. the 404 dispatch at line 570), so keeping it is the safer choice.
Nice work — this matches Next.js behavior and closes a genuine dev-mode gap.
|
Review posted to PR #2401. Summary of findings: Review verdict: No blocking issues — solid, well-scoped fixWhat the PR does: Adds a shared Key correctness points I verified:
Verification run locally: targeted SSR/API tests pass, prod end-to-end test passes, and Minor non-blocking notes (no changes requested): the |
Summary
Tests