fix(pages): preserve gSSP headers on redirects - #2771
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 |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve gSSP headers on redirects
Reviewed against the Next.js oracle (render.tsx retains the shared response object when encoding redirect metadata). The fix is correct and both the dev shortcut and prod Fetch paths are covered. Tests pass, types/lint clean, and the e2e regression asserts both location and surrogate-control. No blocking issues — a few notes below.
Verified
- Prod path is clean.
PagesResponseStream.resHeadersstarts empty and is not seeded from request headers (pages-node-compat.ts:251), sogetPagesGsspResponseHeaders(res)returns only headers gSSP set oncontext.res— exactly the Next.js semantics. Middleware headers are merged separately afterrenderPage()returns, so there's no double-application. buildNextDataPropsJsonResponseContent-Type is now authoritative. Switching tonew Headers(init?.headers)+headers.set("Content-Type", "application/json")means a gSSP-suppliedcontent-typecan no longer override the JSON envelope type for_next/data. The old spread order let it win. This is an improvement (the_next/dataenvelope must be JSON), just noting it's a deliberate behavior change.- set-cookie handling stays consistent.
getPagesGsspResponseHeadersusesappendper array item; forset-cookiethat's correct multi-header behavior, and for other arraysappendon a freshHeadersis equivalent to thejoin(", ")used byapplyGsspHeaders. - README/fixme count is consistent: 13
test.fixmeremain inpages-router-complex, matching the "60/73 pass, 13 gaps" update.
Notes / nits (non-blocking)
-
Dev vs prod header-scope asymmetry. On the normal dev render path, only headers newly set by gSSP are forwarded (via the
headersBeforeGSSPdiff atdev-server.ts:1188/1294). The newwriteGsspRedirectpath forwards all headers currently onres(dev-server.ts:330-334), which in dev includes middleware headers already present onres. In practicewriteHeadwould emit those middleware headers anyway (they're already set viasetHeader, and the explicit keys override on merge), so this is redundant rather than wrong — but it's a slightly different mental model than the prod path, which only ever sees gSSP-set headers. Worth a one-line comment noting the two paths capture different scopes but converge to the same emitted set, so a future reader doesn't "fix" the perceived inconsistency. -
getPagesGsspResponseHeadersduplicates logic inapplyGsspHeaders.pages-page-response.ts:461already maps aPagesGsspResponseinto aHeaders, with the same set-cookie special-casing. Consider consolidating into one shared helper so the redirect path and the normal render path can't drift (e.g. if someone later adds header filtering to one but not the other). Not required for this PR.
Result
LGTM. Correct, well-tested, and preserves dev/prod parity.
|
Review posted to PR #2771. SummaryI reviewed PR #2771 ( Verdict: LGTM — no blocking issues. Key verifications:
Non-blocking notes I flagged:
|
# Conflicts: # examples/pages-router-complex/README.md
Summary
context.reswhengetServerSidePropsreturns a redirectNext.js oracle:
packages/next/src/server/render.tsxretains the shared response object while encoding redirect metadata, so headers set before returning the redirect remain on the final response. Verified directly against Next.js 16.2.7 with the same fixture.Tests
vp check --fix packages/vinext/src/server/dev-server.ts packages/vinext/src/server/pages-data-route.ts packages/vinext/src/server/pages-page-data.ts tests/pages-page-data.test.ts tests/e2e/pages-router-complex/gallery-guards.spec.tsvp test run tests/pages-page-data.test.tsvp run vinext#buildPLAYWRIGHT_PROJECT=pages-router-complex pnpm run test:e2e -- gallery-guards.spec.ts --reporter=line