fix(pages-router): default Cache-Control on gssp responses#1574
fix(pages-router): default Cache-Control on gssp responses#1574james-elicx wants to merge 1 commit into
Conversation
getServerSideProps responses now ship the Next.js default
`private, no-cache, no-store, max-age=0, must-revalidate` Cache-Control
header in both dev and prod, and forward `res.setHeader('Cache-Control', ...)`
overrides set inside gssp through to the final response.
Mirrors next/src/server/route-modules/pages/pages-handler.ts where a
gssp page resolves to `revalidate: 0` and getCacheControlHeader emits
the same string, gated by `if (!res.getHeader('Cache-Control'))` to
preserve user overrides.
Closes #1461
|
/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.
Clean, well-scoped fix. Both code paths (dev-server + pages-page-response) are covered, the priority cascade (user override > nonce > ISR > gSSP default) is correct, and the tests exercise both the default and the override in dev and prod.
Two minor observations — neither blocks merge:
-
Duplicate default in dev-server:
dev-server.ts:592-598sets the defaultCache-ControlintogsspExtraHeaders, then thestreamPageToResponsepath spreadsgsspExtraHeadersintoextraHeadersat line 1070-1072. That works, but the same default is also applied insiderenderPagesPageResponseat line 392-396 (the prod path). For dev, this means the default could theoretically be set twice — once ingsspExtraHeadersand once inrenderPagesPageResponse(sincerenderPagesPageResponseis not used in the dev streaming path, this doesn't actually double-fire today). Worth a comment noting that the dev-server addition is dev-only (streaming path) while thepages-page-response.tsaddition covers the prodrenderPagesPageResponsepath, so they don't overlap. The existing comments are close but don't make this fully explicit. -
Fixture typing:
ssr-cache-control.tsx:18uses{ res: any }for the gSSP context. Minor, but the existingssr-headers.tsxfixture does the same, so it's consistent. Not a blocker.
Tests are thorough — covers dev default, dev override, prod default, and prod override. LGTM.
|
Approved PR #1574 with review. Here's the summary: Verdict: APPROVE The fix correctly adds the default Both code paths are covered:
Tests cover all four scenarios: dev default, dev override, prod default, and prod override. Two minor non-blocking observations were noted about the dev/prod path separation clarity and fixture typing consistency. |
1 participant
Closes #1461
Summary
getServerSidePropsresponses now set the defaultCache-Control: private, no-cache, no-store, max-age=0, must-revalidateunless overridden, and forwardres.setHeader('Cache-Control', ...)values to the final response.Test plan
vp checkpasses