Repository navigation
perf(app-router): run a dynamicParams = false route's generators only on a cache miss - #4634
james-elicx wants to merge 3 commits into
Conversation
…aticParams in production A route handler that also exports POST, PUT, DELETE, PATCH or OPTIONS is never cached, so the check ran its generator on every request only to find the one case Next.js 404s: a generator that returns no params. Once it lists a param, Next.js serves every param, so production now serves these handlers without running the generator. Dev still checks them.
james-elicx
changed the title
|
/bigbonk review for issues Please review exact head |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| if (replace || (route === undefined && nextRoute !== undefined)) { | ||
| route = nextRoute; | ||
| onRoute?.(nextRoute); | ||
| if (nextRoute !== undefined) onRoute?.(nextRoute); |
There was a problem hiding this comment.
The new reset updates this function’s local route to undefined, but deliberately skips onRoute. That callback is how app-response-stage-entry.ts retains the latest route when callback throws (the destructuring assignment never runs in that case). If routing rejects a dynamic-param match, resets, and then a fallback rewrite/render throws, the generated 500 therefore carries the rejected route in X-Vinext-Trace-Route. Notify the callback on resets too (and widen its parameter to string | undefined) so the error path cannot retain stale trace metadata.
There was a problem hiding this comment.
Fixed in 63941a9: captureFrameworkRequestRoute now reports every route change to onRoute, including the reset, and its parameter is string | undefined, so a response stage whose resumed render throws no longer names the rejected route. Covered in tests/request-tracing.test.ts.
|
Review: 1 findings. Posted 1 actionable inline warning. |
|
Performance benchmarksCompared 0 improved · 0 regressed · 7 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
87795d3 to
94d9531
Compare
|
/bigbonk review for issues Please review exact head |
| setCurrentFetchRevalidate(null); | ||
| setCurrentForceDynamicFetchDefault(false); | ||
| // Routing discards this response, so it marks nothing for the request. | ||
| if (options.resumeRoutingAfterMiss?.()) return new Response(null, { status: 404 }); |
There was a problem hiding this comment.
setHeadersContext(requestHeadersContext) immediately above clears draftModeCookieHeader (headers.ts:454-462). If middleware called draftMode().disable(), isDraftMode is false, so an unlisted param reaches this new handoff; fallback routing then proceeds after the outgoing disable cookie was erased, and the client remains in draft mode. Preserve and restore the pending draft transition across this response discard, as the route-handler path does.
There was a problem hiding this comment.
Checked this one: dispatchAppPage runs inside runWithFetchDedupe, which gives the dispatch a shallow child of the unified request state, so setHeadersContext there clears only the child's draftModeCookieHeader. The handler's state keeps middleware's pending header and the resumed dispatch inherits it. 998601f adds coverage for it (middleware draftMode().disable() with and without force-static, asserting the pending disable cookie survives the resume); the assertion holds without any change to the dispatch, so I left the code as is.
|
Review: 1 findings. Posted 1 actionable inline warning. |
94d9531 to
f8d7c70
Compare
|
/bigbonk review for issues Please review exact head |
| }); | ||
| setRootParams(null); | ||
| match = null; | ||
| return finishRouting(); |
There was a problem hiding this comment.
The resumed pass starts from the rejected route's already-rewritten cleanPathname but replays the fallback list from index zero. An earlier rule that did not match the original URL can now match this intermediate destination and incorrectly claim the request. For example, with rules /bridge/:slug -> /final/:slug followed by /start/:slug -> /bridge/:slug, a rejected /bridge/[slug] match for /start/x is incorrectly rerouted to /final/x; sequential fallback routing has already passed the first rule. Resume after the rule that produced the rejected match, or restart from the original routing state while excluding that match.
There was a problem hiding this comment.
Fixed in 92af6d0. Routing now records the fallback rewrite that produced the match and resumes at the next one, keeping the rewritten URL. Next.js does the same: after NoFallbackError it routes again and skips the output it already invoked, so a later rule sees the rewritten pathname and an earlier one never matches it. Because each resume starts at a later rule, routing can resume more than once and still ends when the list runs out. Tests: "resumes routing after the fallback rewrite that reached the rejected route" (your /bridge case) and "resumes routing at each later fallback rewrite".
| params: {}, | ||
| }); | ||
| setRootParams(null); | ||
| match = null; |
There was a problem hiding this comment.
Clearing only match leaves isInterceptionMatch/isFinalRouteMatch true from a rejected interception source. If the fallback rewrite then reaches a Pages-only route, renderPagesForMatchKind() still sees the stale finality at line 2545, skips renderPagesFallback, and returns 404. Reset or recompute the interception-derived finality before rerouting.
There was a problem hiding this comment.
Fixed in 92af6d0. An intercepted match no longer resumes. Routing ends at the intercepted route (isFinalRouteMatch, the same finality that already skips afterFiles and fallback rewrites for it), so its miss keeps the dispatch's 404 and never sees the stale finality. The PR body lists this as a limitation. Test: "doesn't resume routing for an intercepted route".
| shouldResumeRoutingAfterMiss = false; | ||
| // The rejected match no longer names the request. | ||
| resetFrameworkRequestRoute(); | ||
| resetRouteCacheability(); |
There was a problem hiding this comment.
The resume reset leaves the rejected route's cache-bypass state latched. A normalized/raw-path Route Handler can set bypassInterceptionContextCache and the outer interceptionResponseUncacheable, reject its params, and then fall back to a cacheable page whose own route check would not set either flag. That page consequently skips ISR and line 3497 replaces its public policy with never-cache. Restore the pre-dispatch interception/cache-bypass baseline before the resumed finishRouting().
There was a problem hiding this comment.
Fixed in 92af6d0. Before applying a match's own bypass, routing records bypassInterceptionContextCache and the outer uncacheable flag. The handler now tracks that flag locally and reports it outward. Resuming restores both, and the resumed pass works them out again for the new match: fallback rewrites still invalidate the interception proof, and the raw-spelling check runs for the new match. Test: "drops the rejected route's cache bypass when routing resumes" (a route handler match with a raw-spelling bypass resumes to a cacheable page).
|
Review: 3 findings. Posted 3 actionable inline warnings. Targeted tests passed: 784 tests across 5 files. |
f8d7c70 to
92af6d0
Compare
|
/bigbonk review for issues Please review exact head |
| } | ||
| const rewrittenPublicFileResponse = servePublicFileIfMatched(); | ||
| if (rewrittenPublicFileResponse) return rewrittenPublicFileResponse; | ||
| match = matchCleanPathname(); |
There was a problem hiding this comment.
This rematches every route from the rewritten pathname, including the route output that just rejected its params. A later fallback can therefore re-enter that same output with different params and serve it: for example, /start/:slug -> /closed/:slug followed by /closed/:slug -> /closed/listed lets a rejected /closed/[slug] request serve listed. Next.js keeps an invokedOutputs set across the retry and checkTrue() skips dynamic routes whose route.page was already invoked, so that output cannot claim the request again. Track rejected route outputs across resumes and exclude them from matchCleanPathname() (while still allowing later rewrites to reach other routes).
There was a problem hiding this comment.
Fixed in c106ea0. Routing now keeps the routes a dispatch rejected across resumes, and a resumed fallback rewrite that matches one of them leaves the request unmatched. Like checkTrue() skipping an invoked route.page, this holds whatever the params, so later rewrites can still reach other routes, including Pages routes. Test: "doesn't route a resumed request back to the rejected route" (your /closed/listed case).
|
Review: 1 findings. Posted 1 actionable inline warning. Prior findings are fixed. |
92af6d0 to
c106ea0
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
8365884b.
This review, of c106ea00
💬 Approved with comments — One WARNING: in apps with fallback rewrites, a route-tree prefetch for a param the route doesn't list now gets the rejected route's tree, even when a fallback rewrite claims that URL. There is also one INFO about the docs.
Findings: 1 warning · 1 info (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #4634.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] A route-tree prefetch for a rejected param that a fallback rewrite claims now returns the rejected route's tree instead of the fallback target's.
- **File:** packages/vinext/src/server/app-rsc-handler.ts:2975
## INFO (1)
### [Documentation] The docs say a fallback rewrite can claim a rejected param without noting that the split request/response-stage path still answers 404.
- **File:** docs/reference/differences.mdx:76
📋 Code Quality: ⚠️ 1 warning
A route-tree prefetch skips fallback rewrites for a rejected dynamicParams = false param.
Recommendation: 📝 Minor Comments
- [WARNING] A route-tree prefetch for a rejected param that a fallback rewrite claims now returns the rejected route's tree instead of the fallback target's. (
packages/vinext/src/server/app-rsc-handler.ts:2975)
View full review
Route-tree prefetch now ignores fallback rewrites (packages/vinext/src/server/app-rsc-handler.ts:2975)
Before this change, in an app with fallback rewrites, routing ran the generators for a /_tree prefetch. A rejected param fell through to the fallback rewrites, so a URL that a rewrite claims got the rewrite target's tree.
Now the tree is built from route as soon as the route matches. The dispatch never runs, so routing never resumes. For the same URL:
- the
/_treeprefetch returns the rejected[slug]route's segments; - the content request resumes routing and renders the fallback target.
The client can end up holding a route tree for a different route than the one it navigates to.
The test that covered tree prefetches with fallback rewrites was removed, and the new fallback-rewrite tests have no /_tree case.
Two possible fixes:
- Keep the short-circuit only when
options.configRewrites.fallbackis empty or the route isn'tdynamicParams = false. - Or document that tree prefetches ignore fallback rewrites.
Add a /_tree test with a claiming fallback rewrite either way.
📝 Documentation: 💬 1 info
The fallback-rewrite statement doesn't mention the split request/response-stage limitation.
Recommendation: 💬 Optional Nits
- [INFO] The docs say a fallback rewrite can claim a rejected param without noting that the split request/response-stage path still answers 404. (
docs/reference/differences.mdx:76)
View full review
docs/reference/differences.mdx:76says afallbackrewrite can still claim a param thatdynamicParams = falserejects. It doesn't say that the split request/response-stage path still answers 404 for that miss, a limitation the pull request description lists.
✅ All other checks passed
🔒 Security: ✅ LGTM
- Resumed routing: a rejected route can't be routed back into, because
rejectedRoutespersists across resumes. The cache-bypass and interception-uncacheable flags are restored before routing resumes. - No render for a rejected param: the dispatch checks params on every cache miss, so a rejected param never reaches a page or handler render. Server Action requests and intercepted routes don't resume.
- Draft mode: the restore only puts back a
Set-Cookiethe dispatch had taken. - Mixed-method route handlers: skipping the generator in production is a documented conformance difference, not an access-control boundary.
📖 AGENTS.md: ✅ LGTM
This pull request doesn't change tooling or structure in a way that calls for an AGENTS.md update.
⚡ Performance: ✅ LGTM
Cache hits and /_tree prefetches no longer run the generators. The resume loop ends because the fallback rewrite index only moves forward. The discarded 404 body is cancelled before routing resumes.
Reviewed commit: c106ea00
| // Next.js 404s a dynamicParams = false miss's route tree. vinext doesn't run | ||
| // the route's generators for each prefetched link: the tree holds only the | ||
| // route's segments, and the prefetch of its content gets the 404. | ||
| if (isRouteTreePrefetchRequest(request) && !route.routeHandler) { |
There was a problem hiding this comment.
A route-tree prefetch for a rejected param that a fallback rewrite claims now returns the rejected route's tree instead of the fallback target's.
This short-circuit answers /_tree from the matched route before the dispatch can reject the param and resume routing with the fallback rewrites. In an app with fallback rewrites, a prefetch for a param that dynamicParams = false rejects gets the rejected route's segments. The content request for the same URL renders the fallback target, so the client can hold a tree for a different route than the one it navigates to. Before this change, the routing-time check sent such a request to the fallback rewrite. Consider skipping the short-circuit for dynamicParams = false routes when options.configRewrites.fallback is non-empty, or document the difference and add a /_tree test with a claiming fallback rewrite.
There was a problem hiding this comment.
Fixed in e528191. When a fallback rewrite could still claim the URL, the tree's check runs again. On a miss it now resumes routing instead of answering 404, so a claiming fallback rewrite answers with its own route's tree, as in Next.js, and an unclaimed URL gets the unmatched route's 404. The tree is answered from the route alone only when no fallback rewrite is left, or for an intercepted route. Tests: "answers a route-tree prefetch with the tree of the fallback rewrite that claims the URL", "answers a route-tree prefetch that no fallback rewrite claims with the unmatched route's 404" and "answers a route-tree prefetch from the route alone without fallback rewrites". The docs and PR body describe the split.
| Next.js also treats an otherwise static route as SSG when a layout above its last dynamic segment returns every param from `generateStaticParams`. vinext doesn't run the generator to classify requests, so it renders such a route on every request unless it exports `dynamic = "force-static"` or `"error"`. Adding `generateStaticParams` to the last dynamic segment, even one that returns `[]`, makes the route static in both. | ||
|
|
||
| Under `dynamicParams = false`, Next.js checks a request against the params its build generated and runs no `generateStaticParams` per request. For a route the check applies to, vinext runs the route's generators when the cache doesn't answer a request, including a request for params the route doesn't list. In an app with `fallback` rewrites, it runs them while routing for every request to such a route, outside draft mode, so a rewrite can claim a param the route doesn't list, as in Next.js. A child segment's generator runs only for the parent params that match the request, but a generator in a parallel route slot runs for every combination its parents generate. If a child's generator returns a different value for a parent param, vinext runs it for every parent combination once it sees that change. If the generator changes a parent param only for parent values that don't match the request, vinext can miss the change and answer 404 where Next.js renders the page. A cached image metadata response is served without this check, and so is a cached page or route handler response in an app without `fallback` rewrites, so with a pinned `generateBuildId` and a cache that persists across deploys, an entry an earlier deploy stored can outlive the removal of its params. Keep generators cheap, for example by caching the data they fetch. | ||
| Under `dynamicParams = false`, Next.js checks a request against the params its build generated and runs no `generateStaticParams` per request. For a route the check applies to, vinext runs the route's generators when the cache doesn't answer a request, including a request for params the route doesn't list. A page's route-tree prefetch for params the route doesn't list gets the route's tree, which holds only its segments, where Next.js answers 404. The prefetch of the page's content gets the 404. A `fallback` rewrite can still claim a param the route doesn't list, as in Next.js. A route handler that exports a method other than `GET` or `HEAD` runs no generator in production and serves every param, as Next.js does once its generator lists one. When that generator lists none, Next.js answers 404 for every param. A child segment's generator runs only for the parent params that match the request, but a generator in a parallel route slot runs for every combination its parents generate. If a child's generator returns a different value for a parent param, vinext runs it for every parent combination once it sees that change. If the generator changes a parent param only for parent values that don't match the request, vinext can miss the change and answer 404 where Next.js renders the page. A cached page, route handler or image metadata response is served without this check, so with a pinned `generateBuildId` and a cache that persists across deploys, an entry an earlier deploy stored can outlive the removal of its params. Keep generators cheap, for example by caching the data they fetch. |
There was a problem hiding this comment.
ℹ️ INFO · Documentation
The docs say a fallback rewrite can claim a rejected param without noting that the split request/response-stage path still answers 404.
This paragraph says a fallback rewrite can still claim a param the route doesn't list. The split request/response-stage path has no route modules, though, so it still answers that miss with a 404. The pull request description lists this as a limitation. Consider adding that caveat here so users of that deployment shape aren't misled.
There was a problem hiding this comment.
Fixed in e528191. The paragraph now says that on Cloudflare with the Workers Response Store or Workers Cache, the URL gets the 404 instead, because routing runs in a stage without the route's modules. Both adapters route every request through that request stage.
… resuming routing on a miss With fallback rewrites, routing ran a dynamicParams = false route's generators for every request before the cache could answer it. The dispatch now checks the params only on a cache miss and, when a fallback rewrite could claim the URL, resumes routing with the fallback rewrites once, as Next.js does when its route rejects the params.
797fe8e to
e528191
Compare
|
clanky review |
|
/bigbonk review for issues Please review exact head |
There was a problem hiding this comment.
Superseded by the review of
e528191d.
This review, of 8365884b
💬 Approved with comments — The earlier route-tree prefetch warning is fixed, and nothing else in the code needs a change. One INFO remains: the docs don't list the cases where a miss still answers 404.
Findings: 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📝 Documentation: 💬 1 info
The docs don't list the cases where a fallback rewrite doesn't claim a rejected param.
Recommendation: 💬 Optional Nits
- [INFO] The docs say a fallback rewrite can claim a rejected param without listing the cases where the miss still answers 404. (
docs/reference/differences.mdx:76)
View full review
docs/reference/differences.mdx:76 says a fallback rewrite can still claim a param the route doesn't list. In these cases the miss still answers 404:
- the split request/response-stage path;
- an intercepted route;
- a Server Action or progressive-action render.
✅ All other checks passed
🔒 Security: ✅ LGTM
- Rejected params: a rejected param still never renders. The dispatch checks on every cache miss and returns a bare 404 before any handler or page runs. Server Action renders and intercepted routes don't resume.
- Resumed routing:
rejectedRoutespersists across resumes, so a later rewrite can't route the request back into a rejected route. - Cache-bypass flags: they are restored before routing resumes, then worked out again for the new match.
- Draft mode:
restoreDraftModeCookieHeaderonly puts back aSet-Cookiethe dispatch itself took. - Mixed-method route handlers: skipping the generator in production is a documented difference from Next.js, not an access-control boundary.
📖 AGENTS.md: ✅ LGTM
This pull request doesn't change tooling or project structure in a way that calls for an AGENTS.md update.
📋 Code Quality: ✅ LGTM
- Route-tree prefetch with fallback rewrites: fixed. When
canResumeRoutingAfterMiss()is true, a/_treeprefetch runsisDynamicParamsMiss. On a miss it resumes routing, so a claiming rewrite answers with its own route's tree.tests/app-rsc-handler.test.tscovers the claiming, unclaimed and no-fallback cases. - Trace route reset:
resetFrameworkRequestRouteand thereplacehandling inrequest-tracing.tskeep first-wins behaviour for calls that don't replace. A reset is now reported toonRoute.
⚡ Performance: ✅ LGTM
- Cache hits and route-tree prefetches that no
fallbackrewrite could claim no longer rungenerateStaticParams. - The resume loop ends, because
nextFallbackRewriteIndexonly moves forward. - The discarded 404 body is cancelled before routing resumes.
Reviewed commit: 8365884b
| Next.js also treats an otherwise static route as SSG when a layout above its last dynamic segment returns every param from `generateStaticParams`. vinext doesn't run the generator to classify requests, so it renders such a route on every request unless it exports `dynamic = "force-static"` or `"error"`. Adding `generateStaticParams` to the last dynamic segment, even one that returns `[]`, makes the route static in both. | ||
|
|
||
| Under `dynamicParams = false`, Next.js checks a request against the params its build generated and runs no `generateStaticParams` per request. For a route the check applies to, vinext runs the route's generators when the cache doesn't answer a request, including a request for params the route doesn't list. In an app with `fallback` rewrites, it runs them while routing for every request to such a route, outside draft mode, so a rewrite can claim a param the route doesn't list, as in Next.js. A child segment's generator runs only for the parent params that match the request, but a generator in a parallel route slot runs for every combination its parents generate. If a child's generator returns a different value for a parent param, vinext runs it for every parent combination once it sees that change. If the generator changes a parent param only for parent values that don't match the request, vinext can miss the change and answer 404 where Next.js renders the page. A cached image metadata response is served without this check, and so is a cached page or route handler response in an app without `fallback` rewrites, so with a pinned `generateBuildId` and a cache that persists across deploys, an entry an earlier deploy stored can outlive the removal of its params. Keep generators cheap, for example by caching the data they fetch. | ||
| Under `dynamicParams = false`, Next.js checks a request against the params its build generated and runs no `generateStaticParams` per request. For a route the check applies to, vinext runs the route's generators when the cache doesn't answer a request, including a request for params the route doesn't list. When no `fallback` rewrite could still claim the URL, a page's route-tree prefetch for params the route doesn't list gets the route's tree, which holds only its segments, where Next.js answers 404. The prefetch of the page's content gets the 404. When one could, vinext runs the generators for a route-tree prefetch outside draft mode too. A `fallback` rewrite can still claim a param the route doesn't list, as in Next.js. A route handler that exports a method other than `GET` or `HEAD` runs no generator in production and serves every param, as Next.js does once its generator lists one. When that generator lists none, Next.js answers 404 for every param. A child segment's generator runs only for the parent params that match the request, but a generator in a parallel route slot runs for every combination its parents generate. If a child's generator returns a different value for a parent param, vinext runs it for every parent combination once it sees that change. If the generator changes a parent param only for parent values that don't match the request, vinext can miss the change and answer 404 where Next.js renders the page. A cached page, route handler or image metadata response is served without this check, so with a pinned `generateBuildId` and a cache that persists across deploys, an entry an earlier deploy stored can outlive the removal of its params. Keep generators cheap, for example by caching the data they fetch. |
There was a problem hiding this comment.
ℹ️ INFO · Documentation
The docs say a fallback rewrite can claim a rejected param without listing the cases where the miss still answers 404.
This paragraph says a fallback rewrite can still claim a param the route doesn't list, with no exceptions. The pull request description lists cases where the miss still answers 404: the split request/response-stage path, an intercepted route, and a Server Action or progressive-action render. One sentence naming those cases would keep users of those paths from expecting the fallback rewrite to take over.
There was a problem hiding this comment.
Superseded by the review of
c851f650.
This review, of e528191d
💬 Approved with comments — Nothing in the code needs a change. One INFO remains: the differences page doesn't name every case where a rejected param still gets the 404 instead of being claimed by a fallback rewrite.
Findings: 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📝 Documentation: 💬 1 info
The differences page still omits two cases where a rejected param keeps the 404.
Recommendation: 💬 Optional Nits
- [INFO] The fallback-rewrite sentence still omits intercepted routes and Server Action renders, which keep the 404 for a rejected param. (
docs/reference/differences.mdx:76)
View full review
The updated paragraph now covers the route-tree prefetch difference, the Cloudflare split-stage 404 and the production mixed-method route handler difference.
It still says a fallback rewrite can claim a param the route doesn't list without naming two cases where the request keeps the 404 instead: an intercepted route, and a Server Action or progressive-action render.
✅ All other checks passed
🔒 Security: ✅ LGTM
The only change since the last review is a wording update to docs/reference/differences.mdx, and it has no security impact.
- A rejected param still never renders: the dispatch checks on every cache miss and returns a bare 404 before any handler or page runs.
- A later rewrite can't route the request back into a route that rejected its params.
📖 AGENTS.md: ✅ LGTM
This pull request doesn't change tooling or project structure in a way that calls for an AGENTS.md update.
📋 Code Quality: ✅ LGTM
- Resume loop: it ends, because
nextFallbackRewriteIndexonly moves forward.rejectedRoutespersists across resumes. Cache-bypass flags are restored before routing resumes and worked out again for the new match. - No resume: intercepted routes (
isFinalRouteMatch) and Server Action or progressive-action renders keep the 404. The route-tree prefetch check also excludes intercepted routes. - Request tracing:
resetFrameworkRequestRoutekeeps first-wins behaviour for calls that don't replace, and the reset is reported toonRoute. - Route handlers:
restoreDraftModeCookieHeaderputs back only aSet-Cookiethe dispatch itself took. The discarded 404 body is cancelled before routing resumes. The removedenforce-when-none-generatedpolicy leaves no dead code behind.
⚡ Performance: ✅ LGTM
The only change since the last review is a documentation edit, so it has no performance impact.
- Cache hits no longer run
generateStaticParams. - Route-tree prefetches that no
fallbackrewrite could claim no longer run it either.
Reviewed commit: e528191d
| Next.js also treats an otherwise static route as SSG when a layout above its last dynamic segment returns every param from `generateStaticParams`. vinext doesn't run the generator to classify requests, so it renders such a route on every request unless it exports `dynamic = "force-static"` or `"error"`. Adding `generateStaticParams` to the last dynamic segment, even one that returns `[]`, makes the route static in both. | ||
|
|
||
| Under `dynamicParams = false`, Next.js checks a request against the params its build generated and runs no `generateStaticParams` per request. For a route the check applies to, vinext runs the route's generators when the cache doesn't answer a request, including a request for params the route doesn't list. In an app with `fallback` rewrites, it runs them while routing for every request to such a route, outside draft mode, so a rewrite can claim a param the route doesn't list, as in Next.js. A child segment's generator runs only for the parent params that match the request, but a generator in a parallel route slot runs for every combination its parents generate. If a child's generator returns a different value for a parent param, vinext runs it for every parent combination once it sees that change. If the generator changes a parent param only for parent values that don't match the request, vinext can miss the change and answer 404 where Next.js renders the page. A cached image metadata response is served without this check, and so is a cached page or route handler response in an app without `fallback` rewrites, so with a pinned `generateBuildId` and a cache that persists across deploys, an entry an earlier deploy stored can outlive the removal of its params. Keep generators cheap, for example by caching the data they fetch. | ||
| Under `dynamicParams = false`, Next.js checks a request against the params its build generated and runs no `generateStaticParams` per request. For a route the check applies to, vinext runs the route's generators when the cache doesn't answer a request, including a request for params the route doesn't list. When no `fallback` rewrite could still claim the URL, a page's route-tree prefetch for params the route doesn't list gets the route's tree, which holds only its segments, where Next.js answers 404. The prefetch of the page's content gets the 404. When one could, vinext runs the generators for a route-tree prefetch outside draft mode too. A `fallback` rewrite can still claim a param the route doesn't list, as in Next.js. On Cloudflare with the Workers Response Store or Workers Cache, the URL gets the 404 instead, because routing runs in a stage without the route's modules. A route handler that exports a method other than `GET` or `HEAD` runs no generator in production and serves every param, as Next.js does once its generator lists one. When that generator lists none, Next.js answers 404 for every param. A child segment's generator runs only for the parent params that match the request, but a generator in a parallel route slot runs for every combination its parents generate. If a child's generator returns a different value for a parent param, vinext runs it for every parent combination once it sees that change. If the generator changes a parent param only for parent values that don't match the request, vinext can miss the change and answer 404 where Next.js renders the page. A cached page, route handler or image metadata response is served without this check, so with a pinned `generateBuildId` and a cache that persists across deploys, an entry an earlier deploy stored can outlive the removal of its params. Keep generators cheap, for example by caching the data they fetch. |
There was a problem hiding this comment.
ℹ️ INFO · Documentation
The fallback-rewrite sentence still omits intercepted routes and Server Action renders, which keep the 404 for a rejected param.
This paragraph now names the Cloudflare split-stage case. It still doesn't mention that an intercepted route, or a Server Action or progressive-action page render, keeps the 404 for a param the route doesn't list, even when a later fallback rewrite would claim the URL. A short clause after "as in Next.js" listing those two cases would keep readers from expecting the rewrite to always take over.
There was a problem hiding this comment.
Done in c851f65. The paragraph now also says that a page an intercepted route renders, or a page a Server Action has run on, gets the 404. With the Cloudflare stage case added earlier, it names every case where a fallback rewrite can't claim a rejected param.
…teStaticParams Client navigation prefetches a route tree for each link it sees, and the tree holds only the route's segments. When no fallback rewrite could claim the URL, answering it from the route alone keeps a page's generators off every prefetched link; the prefetch of the page's content still gets the dynamicParams = false 404. When one could, the tree's check still runs, so a claiming fallback rewrite answers with its own route's tree.
e528191 to
c851f65
Compare
|
/bigbonk review for issues Please review exact head |
There was a problem hiding this comment.
✅ Approved — The earlier documentation note is fixed and nothing new turned up: the resume logic, cache-bypass reset and route-tree prefetch handling are sound, and the updated docs match the behaviour.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
A rejected param still never renders. A later rewrite can't route the request back into a route that rejected its params.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md update is needed.
📋 Code Quality: ✅ LGTM
- Resume loop: it ends, because
nextFallbackRewriteIndexonly moves forward.rejectedRoutespersists across resumes, and the cache-bypass flags are restored before each resume. - Route handler miss: a draft-mode
Set-Cookiethe dispatch took is put back throughrestoreDraftModeCookieHeaderbefore the bare 404 is returned. - Dead code: removing the
enforce-when-none-generatedpolicy leaves no references behind.
⚡ Performance: ✅ LGTM
Cache hits no longer run generateStaticParams. Route-tree prefetches that no fallback rewrite could claim no longer run it either.
📝 Documentation: ✅ LGTM
The paragraph about fallback rewrites in docs/reference/differences.mdx now names intercepted routes and pages a Server Action has run on, as well as the Cloudflare split-stage case.
Reviewed commit: c851f650
|
@james-elicx Bonk workflow timed out. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
Stacked on #4125.
The two PRs below run a
dynamicParams = falseroute'sgenerateStaticParamsper request, because vinext has no build-time params list to check against. This PR makes the generators run only when the cache can't answer the request.What changes
fallbackrewrites ran the generators before the cache, on every request. It's gone. The page and route handler dispatches already check after their cache read. On a miss, the dispatch now returns a bare 404 without rendering anything. Routing discards it and resumes with the fallback rewrites after the one that reached the rejected route. Next.js does the same when a route rejects its params (NoFallbackError): it routes again, skipping the route it already tried.fallbackrewrite could claim the URL, a/_treeprefetch no longer runs the generators. vinext builds the tree from the route definition only, so it holds the route's segments and nothing rendered for the param. The prefetch of the page's content still gets the 404. Next.js answers 404 for the tree itself, so this is a documented difference. When a fallback rewrite could claim the URL, the tree's check still runs. On a miss, routing resumes, so a claiming rewrite answers with its own route's tree, as in Next.js, and an unclaimed URL gets the unmatched route's 404.dynamicParams = falseroutefallbackrewrites/_treeprefetch (one for each link the client sees), nofallbackrewrite left to claim it/_treeprefetch, app withfallbackrewritesKeeping the 404 behaviour
generateMetadata, page or handler runs.Set-Cookieit took, so the fallback response carries it.?__vinext_rewrite=, so it is separate from the rejected path's entry.Limitations
fallbackrewrites, as it did before. That stage has no route modules.mainnever did.