fix(app-router): cache default static pages#2537
Conversation
|
/bigbonk review for issues |
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 |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — fix(app-router): cache default static pages
I reviewed the diff against main, traced the read/write/response paths, verified Next.js parity for the sentinel, and ran the targeted suites (app-page-*, isr-cache, isr-decision, cache-control, cdn-cache, cloudflare-cdn-cache, kv-cache-handler, app-router-production-server). Everything passes and vp check is clean on all touched source files. The core fix is correct and well-tested.
What the change gets right
- Sentinel parity.
INFINITE_CACHE_SECONDS = 0xfffffffematches Next.js'sINFINITE_CACHEinpackages/next/src/lib/constants.tsexactly (chosen because it is a V8 SMI and JSON-serializable). Good call routing indefinite revalidation through it so KV/serialized adapters preserve the policy (kv-cache-handlerround-trip test confirms"revalidate":4294967294). - Consistent sentinel handling on read. Every place that builds
Cache-Controlfrom a stored revalidate now special-cases bothInfinityandINFINITE_CACHE_SECONDS(cache-control.ts:98,isr-decision.ts:104,isr-decision.ts:172). The huge finite value also degrades gracefully inMemoryCacheHandler—revalidateAt = now + 0xfffffffe*1000(~136 years) is effectively never-stale, matching intent. - Response-policy ordering preserved. In
resolveAppPageRscResponsePolicy/resolveAppPageHtmlResponsePolicytherevalidateSeconds === nullbranch correctly keeps force-static / dynamic-error asSTATICwhile promoting the default (neither) case toMISSin production. The dynamic-usage precedence check still runs first, so late dynamic bailouts stayno-store. - Late-dynamic CDN header safety. The new
dynamicUsageCheckCompleteplumbing is sound: on edge / non-origin-revalidating adapters the code settles the render (RSC path) or buffers HTML (HTML path) to prove non-dynamic before emitting cacheableCDN-Cache-Control. When dynamic is detected late,scheduleAppPageRscCacheWriteearly-returns and theno-storepolicy is preserved — covered bydoes not expose cacheable CDN headers when default-config HTML becomes dynamic late.
Things worth confirming (non-blocking)
-
HTML buffering trades away streaming for edge default-static pages.
app-page-render.ts:1097fully buffers the HTML viareadStreamAsTextand re-wraps it (new Response(bufferedHtml).body!) wheneverisProduction && !dynamicUsedDuringRender && shouldCaptureRscForCacheMetadata && (isEdgeRuntime || !ownsBackgroundRevalidation). For the default Node adapter (ownsBackgroundRevalidation = true, non-edge) this block is skipped and the existingshouldSpeculativelyWriteCachepath handles it without buffering, so common Node deploys keep streaming. But on Cloudflare Workers (edge) every config-less static page now buffers its whole body in memory before the first byte is sent. This is a defensible tradeoff (the CDN can't background-revalidate at the origin, so dynamic usage must be resolved before cacheable headers escape, and Next.js likewise fully renders during static generation), but it is a real change to first-byte latency / memory for those pages. Confirm this is intentional and consider a short code comment on the buffering block explaining why edge must buffer, since the guard condition is subtle. -
readStreamAsTextdecode/re-encode round-trip. Buffering goes bytes → UTF-8 text → bytes.readStreamAsTextusesdecoder.decode(value, { stream: true })so multi-byte sequences split across chunks are handled correctly, and HTML is always UTF-8 here, so this is safe. Flagging only because it's easy to miss that the body is now decoded/re-encoded rather than passed through. -
Dev RSC default-static policy change.
resolveAppPageRscResponsePolicyforrevalidateSeconds === nullin dev now returns{ cacheControl: STATIC_CACHE_CONTROL, cacheState: "STATIC" }instead of the previous{}. This is harmless (dev responses aren't CDN-cached and nothing writes the cache in dev) and matches what the HTML policy already did, but there's no unit test pinning the dev branch. A one-line assertion would lock it in.
Verdict
The fix correctly restores Next.js static-generation semantics (default → indefinitely cacheable, demoted only by explicit config or dynamic usage) and closes a real bug where fully static pages never produced reusable artifacts. Test coverage is strong across unit + production-server layers. No correctness blockers found. The buffering tradeoff (#1) is the only item I'd want an explicit sign-off / comment on before merge.
|
Review posted to PR #2537. Summary of my findings: Review outcome: No correctness blockersThe fix correctly restores Next.js static-generation semantics — App Router pages with no explicit What's solid:
Non-blocking items I flagged for author sign-off:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — fix(app-router): cache default static pages
I traced the read/write/response/finalizer paths, checked dev/prod parity, verified the Next.js sentinel, and ran the targeted suites (app-page-dispatch, app-page-response, app-page-cache, app-page-render, isr-cache, isr-decision, cache-control, kv-cache-handler, cdn-cache, cloudflare-cdn-cache, and the app-router-production-server static tests). Everything passes and vp check is clean on all seven changed source files. The fix is correct and I found no correctness blockers.
What I verified
- Sentinel parity.
INFINITE_CACHE_SECONDS = 0xfffffffematches Next.js'sINFINITE_CACHE. It is consistently special-cased alongsideInfinityin every read-path builder (cache-control.ts:98,isr-decision.ts:104,isr-decision.ts:172) and round-trips through KV serialization (kv-cache-handlertest confirms"revalidate":4294967294). - cacheLife-only routes are unaffected by the read-seed change.
resolveAppPageCacheReadRevalidateSecondschanged thenullseed from0→Infinity. I confirmed this is behavior-preserving for regeneration:resolveRegeneratedAppPageCachePolicy(app-page-cache.ts:208-214) previously tookrenderRevalidateSecondsbecause0 > 0was false; now it takesMath.min(Infinity, renderRevalidateSeconds)= the same value. HIT cache-control still derives from the storedcacheControlMeta.revalidate ?? seed, so acacheLifeentry keeps its own duration. - Write policy default.
resolveAppPageCacheWritePolicynow defaultsnull→INFINITE_CACHE_SECONDSand only returnsnullfor NaN/≤0.Math.min(INFINITE_CACHE_SECONDS, requestCacheLife.revalidate)correctly narrows to the explicit cacheLife value. This is the core fix — config-less static pages now produce reusable artifacts instead of being demoted torevalidate: 0. - Late-dynamic CDN safety. The
dynamicUsageCheckCompleteplumbing is sound. On edge / non-origin-revalidating adapters, the RSC path settles the captured render and the HTML path buffers viareadStreamAsTextto prove non-dynamic beforependingDynamicCheckflips tofalseand cacheableCDN-Cache-Controlescapes.settleCapturedRscRenderForCacheMetadatahandles thenullpromise gracefully, and in the RSC branchcapturedRscDataRef.valueis guaranteed set whenevershouldCaptureRscForCacheMetadatais true (sideStream is created under the same condition), so the settle is never a silent no-op there. readStreamAsTextround-trip decodes with{ stream: true }and a final flush, so multi-byte UTF-8 sequences split across chunks are handled — the bytes→text→bytes buffering is safe.
Non-blocking observations
-
Example home page switched to
force-dynamic(examples/app-router-cloudflare/app/page.tsx:3). This is required by the fix, not incidental: the home page rendersnew Date().toISOString()underdata-testid="timestamp", and two E2E tests assert the timestamp differs across requests (tests/e2e/cloudflare-workers/ssr.spec.ts:36,tests/e2e/cloudflare-workers/interactive.spec.ts:69). Withoutforce-dynamic, default-static caching would serve identical timestamps and break both. The change keeps those tests valid. Trade-off: the flagship example no longer demonstrates the new default-static behavior — worth a one-line comment on the export so a future reader doesn't "clean it up" and silently break the E2E suite. -
Edge default-static pages now buffer the full HTML body in memory before the first byte (
app-page-render.ts:1100). This is skipped for the default Node adapter (ownsBackgroundRevalidation), so common Node deploys keep streaming, but Cloudflare Workers now pay first-byte-latency/memory for every config-less static page. The e6d243b commit already added a code comment explaining why edge must buffer — good. Just confirm the latency trade-off is intentional for the edge target. -
Redundant-but-harmless write guard.
app-page-render.ts:1157adds(dynamicUsageCheckComplete && htmlResponsePolicy.cacheState === "MISS"). For the default-null production branch,resolveAppPageHtmlResponsePolicyalready returnsshouldWriteToCache: truewhenevercacheState === "MISS", so the added disjunct never independently triggers a write today. Not a bug — just defensive. Fine to keep. -
Dev RSC default-static branch is untested.
resolveAppPageRscResponsePolicyforrevalidateSeconds === nullin dev now returns{ cacheControl: STATIC, cacheState: "STATIC" }for force-static/dynamic-error and{}for the plain default. Harmless (dev isn't CDN-cached), but a one-line assertion pinning the dev branch would lock it in.
Verdict
Correct restoration of Next.js static-generation semantics (default → indefinitely cacheable, demoted only by explicit config or dynamic usage), with strong unit + production-server coverage. No blockers. The only items worth an explicit author sign-off are the edge-buffering latency trade-off (#2) and a comment on the example's force-dynamic (#1) so it isn't accidentally reverted.
|
Review posted to PR #2537. SummaryI reviewed the fix that makes App Router pages with no explicit What I verified beyond the prior review:
Notable finding I added: The example home page's switch to Non-blocking items: edge HTML buffering trade-off (already documented in a follow-up commit), a redundant-but-harmless write guard at |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — fix(app-router): cache default static pages
I independently traced the read/write/response/finalizer paths, verified the Next.js sentinel and the null vs Infinity distinction, checked dev/prod parity, and re-ran the targeted suites (app-page-render, app-page-response, app-page-cache, app-page-dispatch = 188 tests; isr-cache, isr-decision, cache-control, kv-cache-handler, cdn-cache, cloudflare-cdn-cache = 206 tests). All pass and vp check is clean on all seven changed source files. The fix is correct — no correctness blockers found.
What I verified beyond the prior reviews
-
Infinitybranches are still live, not dead code. The newrevalidateSeconds === nullbranch handles the default-unspecified case; the existingrevalidateSeconds === Infinitybranch (app-page-response.ts:172,:266) still handles explicitrevalidate = false, which normalizes toInfinityupstream (app-page-dispatch.ts:395,app-route-handler-policy.ts:69). The two cases are genuinely distinct and both covered. -
Write path stays guarded for dynamic pages.
resolveAppPageCacheWritePolicynow seedsnull → INFINITE_CACHE_SECONDS, but the HTML write still bails oncapturedDynamicUsageBeforeContextCleanup()/consumeDynamicUsage()(app-page-cache-finalizer.ts:157-163) and the RSC write bails ondynamicUsedDuringBuild(:263). Default-static pages that turn out dynamic are not written. -
Edge buffering ordering is sound.
readStreamAsText(safeHtmlStream)drains thedeferUntilStreamConsumedwrapper, whoseflush()fires before the read reportsdone, sodynamicUsedBeforeContextCleanup/ context-clear have already run when line 1102 re-reads dynamic usage. The re-wrappednew Response(bufferedHtml).body!is a plain buffered stream, so there is no double-flush or duplicateclearRequestContext()downstream. -
pendingDynamicCheckinversion is correct.dynamicUsageCheckComplete === truesetspendingDynamicCheck: false, which lets the real cacheableCache-Controlescape only after dynamic usage is definitively resolved. Thebecomes dynamic latetest confirmscache-control: no-store+ nocdn-cache-control+ noisrSetwhen late dynamic usage is detected on the Cloudflare adapter. -
Dev default-static branch is now tested. The prior review flagged the dev RSC
nullbranch as untested; the newkeeps default-config pages uncached in developmentcase (app-page-response.test.ts) pins both the RSC ({}) and HTML ({ shouldWriteToCache: false }) dev outcomes. -
Sentinel round-trips + degrades gracefully.
INFINITE_CACHE_SECONDS = 0xfffffffeserializes as4294967294(kv-cache test) and yieldsrevalidateAt = now + ~4.29e9 * 1000(~136 yrs) inMemoryCacheHandler(cache-handler.ts:299-302) — effectively never-stale, matching intent.
Non-blocking (already acknowledged by the author's follow-up commits)
-
Edge HTML/RSC buffering trade-off. Config-less static pages on Cloudflare Workers now fully buffer the body (HTML via
readStreamAsText, RSC via settling the captured render) before the first byte, to prove non-dynamic before cacheable headers escape. This is skipped for the default Node adapter (ownsBackgroundRevalidation), so common Node deploys keep streaming. Commite6d243balready added the explaining comment — the first-byte-latency trade-off for edge is the only thing worth an explicit sign-off. -
Redundant-but-harmless write guard at
app-page-render.ts:1158(dynamicUsageCheckComplete && cacheState === "MISS") never independently triggers a write today because thenull/MISS policy already setsshouldWriteToCache: true. Defensive; fine to keep.
Verdict
Correct restoration of Next.js static-generation semantics (default → indefinitely cacheable via the JSON-safe sentinel, demoted only by explicit config or dynamic usage), with strong unit + production-server coverage and no dev/prod parity regressions. Approve on correctness. Only the edge-buffering latency trade-off (#1) warrants an explicit author sign-off before merge.
|
Review posted to PR #2537. SummaryI independently reviewed the fix that makes App Router pages with no explicit Key things I verified beyond the prior reviews:
Ran targeted suites (394 tests across 10 files) — all pass; |
1 participant
Summary
dynamicorrevalidateconfig as indefinitely cacheable by default, matching Next.js static-generation semanticsNext.js parity
Next.js initializes App Router prerender work with
INFINITE_CACHEand only reduces it through explicit revalidation, uncached fetches, or dynamic request API usage. Vinext instead converted an unspecified route revalidation value to0on cache reads/writes, causing otherwise fully static pages to rerender and never produce reusable artifacts.Validation
vp checkon all changed source and test filesvp test run tests/app-page-*.test.ts tests/isr-cache.test.ts tests/isr-decision.test.ts tests/cache-control.test.ts tests/cdn-cache.test.ts tests/cloudflare-cdn-cache.test.ts tests/kv-cache-handler.test.ts(671 tests)vp test run tests/app-router-production-server.test.ts(68 tests)git diff --check