fix(app-router): default static and SSG routes to revalidate = false - #3452
james-elicx wants to merge 10 commits into
Conversation
|
/bigbonk review for issues |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
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 |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1bf33db to
4f5da5d
Compare
4f5da5d to
bf6a826
Compare
|
/bigbonk review for issues |
|
LGTM! |
c0dad91 to
125c186
Compare
|
/bigbonk review for issues Please review exact head |
|
|
LGTM! |
125c186 to
7c37905
Compare
|
/bigbonk review for issues Please review exact head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| revalidate: effectiveRevalidate, | ||
| // JSON can't hold Infinity, so store `revalidate = false` as false, | ||
| // like Next.js does. Reads turn it back into Infinity. | ||
| revalidate: Number.isFinite(effectiveRevalidate) ? effectiveRevalidate : false, |
There was a problem hiding this comment.
[P2] Avoid giving revalidate = false entries the finite KV TTL. effectiveRevalidate is still Infinity here, so the revalidateAt calculation above produces Infinity and the later revalidateAt !== null check passes ttlSeconds (30 days) to kv.put. JSON turns the stored timestamp into null, but Cloudflare KV will still delete the key after 30 days, so these pages do not remain cached until invalidation/deploy as intended. Treat non-finite revalidate values as having no revalidateAt/expirationTtl, and assert the put options in the round-trip test.
There was a problem hiding this comment.
Fixed in c974d11. revalidateAt is now only set for a finite positive revalidate, so revalidate = false entries get no revalidateAt and no KV expirationTtl. They stay until they're invalidated, like entries stored with no revalidate. The round-trip test now asserts the put options (expirationTtl: undefined).
|
Posted 1 actionable inline finding. Targeted tests and checks passed. |
7c37905 to
c974d11
Compare
|
/bigbonk review for issues Please review exact head |
| typeof effectiveRevalidate === "number" && effectiveRevalidate > 0 | ||
| typeof effectiveRevalidate === "number" && | ||
| effectiveRevalidate > 0 && | ||
| Number.isFinite(effectiveRevalidate) |
There was a problem hiding this comment.
[P2] Keep invalidation markers as long as indefinite entries. This change gives revalidate = false entries no KV TTL, but revalidateTag() markers still expire after 30 days below. If such an entry is invalidated and is not read during that window, the marker disappears while the entry remains; the next read accepts the old entry again and serves invalidated content. Persist these markers without an expiration (newer entries are already protected by the lastModified comparison), or otherwise ensure an invalidated entry cannot outlive its marker, and cover the >30-day case.
There was a problem hiding this comment.
Fixed in 01d284ccc. revalidateTag() markers are now written with no expirationTtl, so an invalidated revalidate = false entry can't outlive its marker. Newer entries still pass the marker by lastModified. The new test stores a revalidate = false entry, invalidates its tag, asserts the marker put has no options, and checks that a fresh handler 31 days later still treats the entry as invalidated.
|
Posted 1 actionable inline finding. Targeted checks passed: 319 tests and focused format/lint/type checks. |
c974d11 to
3d66f3b
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
3d66f3b to
4b874c3
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
4b874c3 to
3827fbd
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
97b0d84 to
6cda46e
Compare
|
/bigbonk review for issues Please review exact head |
1 similar comment
|
/bigbonk review for issues Please review exact head |
| options.isProduction && options.pprRuntime === undefined && isNextStaticEligible; | ||
| const currentRevalidateSeconds = | ||
| options.revalidateSeconds ?? | ||
| (hasStaticRevalidateDefault || options.hasAnyGenerateStaticParams ? Infinity : null); |
There was a problem hiding this comment.
[P1] Let dynamic-fetch observations veto this new cache path. A literal static route now gets Infinity here, so an auth-keyed fetch taking the safety bypass in fetch-cache.ts records dynamicFetchUrls but deliberately leaves consumeDynamicUsage() false. Both HTML and RSC finalizers only test the latter before writing, while readAppPageCacheResponse() does not reject an observation whose downgrade target is freshRender; the first user's auth-derived payload is therefore stored indefinitely and served as a HIT to later users. This also makes response-store admission report the render cacheable because that path consumes but never checks the observation. Please reject page writes/admission when the completed render has dynamic fetch observations (or otherwise feed that signal into dynamic usage), and cover a default-static route whose auth-keyed fetch varies between requests.
There was a problem hiding this comment.
Declining this for Next.js parity. Adding the veto would make vinext store less than Next.js 16.2.7, which stores this render:
- In a runtime (ISR/on-demand) render,
patch-fetch.tssetsautoNoCachefor any fetch with no explicit cache config (L533–547). That givesfinalRevalidate = 0with cacheReason'auto no cache'(L657), so the fetch itself isn't data-cached, but L675–676 skip bothmarkCurrentScopeAsDynamicand the prerender store's revalidate lowering for it:// we don't consider autoNoCache to switch to dynamic for ISR. An auth header only matters when the store is already atrevalidate === 0. - The page's
collectedRevalidate(app-render.tsx) therefore staysfalse, so Next.js stores the page and sendss-maxage=31536000. - vinext's auth-header bypass leaves dynamic usage unset on purpose, to match that
autoNoCachepath (fix(cache): honor fetch opt-outs and force-dynamic revalidate parity #1907: "does not mark pages dynamic"). Vetoing ondynamicFetchUrlswould undo that. It would also veto every default-static page that does a plainfetch(url), since the configless branch records the same observation. - A per-user payload needs
cookies()/headers()or another request API, and those already mark dynamic usage, so neither Next.js nor vinext stores it. An auth-keyed fetch with no request API behind it (e.g. a server secret) is static in Next.js. - This PR doesn't add the path; it widens it. On the parent, routes with
generateStaticParamsor an explicitrevalidatealready stored such renders through the same finalizers and admission. The comments infetch-cache.tsthat suggest the observation downgrades full-page output are misleading and predate this PR; they're noted for a separate follow-up.
|
Posted 1 actionable inline finding. Focused validation passed: 389 tests across 4 files. Worktree remained unchanged. |
6cda46e to
c65e6d6
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
f7f5fa4 to
dffd1ef
Compare
Next.js serves a static or SSG route with no revalidate source until it is revalidated. vinext only defaulted generateStaticParams routes, so a static page without a revalidate export, fetch revalidate or cacheLife was never stored on any backend. Default every static-eligible route to revalidate = false, keep capturing its RSC payload so a cacheLife resolved after headers still lowers the stored lifetime, and send RSC misses that can still turn dynamic as MISS instead of STATIC. force-static and dynamic = "error" keep STATIC.
…ntext The RSC completion marker consumes dynamic usage from the response stream's pull, outside the render's request scope. It memoized false there, so the RSC cache write stored renders that had read a dynamic API.
JSON turned revalidate: Infinity into null, which failed entry validation, so KV never served a static page. Store it as false, like Next.js, and read it back as Infinity.
… page entries Next.js pairs expireTime only with a finite revalidate, so a revalidate = false entry stays until it is invalidated. The App page writers stored the route expireTime with it, and the memory and KV handlers then hard-expired the entry after a year. A cacheLife expire still bounds it.
dffd1ef to
5cb2228
Compare
1 participant
Stacked on #3451. Second PR in the stack that brings ISR query-key handling in line with Next.js.
Problem
Next.js defaults every static or SSG App page to
revalidate = false. It serves such a page withs-maxage=31536000untilrevalidatePath,revalidateTagor the next deploy.vinext only applied that default to
generateStaticParamsroutes. On a route without dynamic segments (or aforce-staticroute) with norevalidateexport, fetchrevalidateorcacheLife,revalidateSecondsstayednull. The finalizer then found no cache policy, so core, the Response Store and Workers Cache never stored the page.Two related gaps on routes that are already
Infinity(generateStaticParamsroutes and explicitrevalidate = false):mayResolveCacheLifeAfterHeaderswas false forInfinity). So an RSC-only miss stored nothing, and acacheLiferesolved after headers couldn't lower thersc:lifetime.STATICfor anyInfinityrender. That made the finalizer keeps-maxage=31536000, even on a miss that reaches a dynamic API late, for examplecookies()under Suspense. The HTML policy already sendsMISSfor the same case.Fix
revalidate = false. Non-eligible and PPR routes keep today's value, and fetch caching is unchanged because the fetch shim treatsnullandInfinitythe same. Literal routes now take theshouldWriteToCachepath thatgenerateStaticParamsroutes already use, instead of the speculative path.mayResolveCacheLifeAfterHeadersis now true forInfinityon static-eligible routes. RSC is captured and stored from the same render, and acacheLiferesolved later still lowers the stored lifetime (min(route revalidate, cacheLife)).MISS: in production, anInfinityRSC render is sent asMISS, so the finalizer applies the pending-dynamic headers, as it does for HTML.STATICfor force-static: onlyforce-staticanddynamic = "error"routes whose revalidate is absent orfalsekeepSTATIC, since they can't turn dynamic while streaming. The HTML policy's force-static branch now accepts the defaultedInfinitytoo.Behaviour change
Static pages with no revalidate source are now cached until they're revalidated, as on Next.js. A page that reads data without a dynamic API, for example a direct database query, now stays cached until
revalidatePath/revalidateTagor the next deploy.Tests
tests/app-page-render.test.ts:revalidate: Infinity;cacheLiferesolved after headers lowers both entries to 60 s;rsc:with thecacheLifelifetime;InfinityRSC miss doesn't send static headers;STATIC.Three of these fail without this change.
tests/app-page-dispatch.test.ts: a literal route with no revalidate source writeshtml:andrsc:withInfinity, and adapter admission reports it cacheable. All three fail without this change.tests/app-page-response.test.ts: the RSC policy matrix.Response Store e2e: a new
/static-defaultdemo page goes MISS → HIT with the same render, and its RSC request hits. 22/22 pass locally.Deployed e2e (
cache-prewarm.spec.ts):/static-defaultis served from one render on the response-store, workers-cache and KV deployments.