fix(pages-router): pass revalidateReason to gssp/getStaticProps context#1573
fix(pages-router): pass revalidateReason to gssp/getStaticProps context#1573james-elicx wants to merge 1 commit into
Conversation
Pages Router `getStaticProps` now receives `context.revalidateReason` matching Next.js: `'build'` during build-time prerendering, `'on-demand'` when the dispatch is signalled as an explicit revalidation, and `'stale'` for stale-while-revalidate background regeneration and runtime cache-miss requests. Closes #1462
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — revalidateReason + gSSP Cache-Control
revalidateReason implementation
The core revalidateReason implementation in pages-page-data.ts is correct. The tri-state resolution logic mirrors Next.js render.tsx faithfully:
"on-demand"whenisOnDemandRevalidateis set"build"whenisBuildTimePrerenderingis set (viaVINEXT_PRERENDER=1)"stale"as the runtime default (cache-miss and SWR background regeneration)
The type narrowing on the getStaticProps context, the isBuildTimePrerendering wiring in the generated entry, and the hardcoded "stale" for SWR background regen paths are all correct. Good tests covering all three branches plus the SWR path.
Two issues to address
1. PR title says "gssp" but the fix is for getStaticProps
The PR title is fix(pages-router): pass revalidateReason to gssp/getStaticProps context, but revalidateReason is only added to getStaticProps, not getServerSideProps. This is correct behavior (Next.js doesn't put revalidateReason on the gSSP context — it's an ISR concept), but the title is misleading. The /getStaticProps part is accurate; the gssp part should be dropped.
2. The gSSP default Cache-Control fix creates a dev/prod parity gap
The diff includes an unrelated fix (referencing #1461) that adds Cache-Control: private, no-cache, no-store, max-age=0, must-revalidate as the default for getServerSideProps responses in the dev server. This is correct behavior (matches Next.js pages-handler.ts), but the corresponding production path does not get the same treatment.
The production flow is: pages-server-entry.ts → resolvePagesPageData() → renderPagesPageResponse() → applyGsspHeaders(). Neither renderPagesPageResponse nor the isDataReq JSON short-circuit path add a default Cache-Control for gSSP responses when the user didn't set one. This means:
- Dev: gSSP responses get
private, no-cache, no-store, max-age=0, must-revalidate✓ - Prod: gSSP responses get no Cache-Control header at all ✗
Per AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as follow-ups — fix them in the same PR."
The fix belongs in pages-page-response.ts's renderPagesPageResponse — add an else branch after the scriptNonce and isrRevalidateSeconds Cache-Control cases for when gsspRes is present and no Cache-Control has been set yet. The isDataReq path in pages-server-entry.ts (lines 645–655) should also get the default.
Minor notes
isOnDemandRevalidateis forward-looking plumbing — no caller sets it today (res.revalidate()is not implemented yet). That's fine as typed infrastructure, but worth noting in a comment so future readers know it's intentional dead code.- The tests are well-structured. The SWR regeneration test that passes
isBuildTimePrerendering: truealongside a stale cache hit is a particularly good edge case — it verifies the SWR path always uses"stale"regardless of the outer dispatch context.
| if (!hasUserCacheControl) { | ||
| gsspExtraHeaders["Cache-Control"] = | ||
| "private, no-cache, no-store, max-age=0, must-revalidate"; | ||
| } |
There was a problem hiding this comment.
This default Cache-Control fix is correct for dev, but the production path (renderPagesPageResponse in pages-page-response.ts) doesn't set any default Cache-Control for gSSP responses. This creates a dev/prod parity gap — dev returns private, no-cache, no-store, ... while prod returns no Cache-Control at all.
The fix should also go in pages-page-response.ts (after the scriptNonce and isrRevalidateSeconds branches) and the isDataReq JSON short-circuit in pages-server-entry.ts.
Also, this is an unrelated fix (referencing #1461) bundled into a PR whose title and description only mention revalidateReason (#1462). Consider either splitting it into its own PR or updating the PR description to mention both fixes.
| // Mirrors Next.js's \`renderOpts.isBuildTimePrerendering\`. See | ||
| // \`.nextjs-ref/packages/next/src/server/render.tsx\` and | ||
| // \`packages/vinext/src/build/prerender.ts\`. | ||
| isBuildTimePrerendering: typeof process !== "undefined" && process.env && process.env.VINEXT_PRERENDER === "1", |
There was a problem hiding this comment.
isOnDemandRevalidate is not passed here, so the "on-demand" branch in resolvePagesPageData is currently unreachable. That's fine as forward-looking plumbing, but consider adding a brief comment noting that on-demand revalidation (res.revalidate()) is not yet wired up — it will save someone from wondering why it's never set.
|
Review posted on PR #1573. Here's a summary of the findings: revalidateReason (the stated fix)The core implementation is correct — the tri-state resolution ( Two issues flagged
|
1 participant
Closes #1462
Summary
getServerSidePropsandgetStaticPropscontext now includesrevalidateReason('build', 'on-demand', or 'stale') matching Next.js.Test plan
vp checkpasses