fix(prerender): surface thrown generateStaticParams/getStaticPaths errors - #2017
Conversation
commit: |
41fed8e to
bed6200
Compare
|
/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: surfacing thrown generateStaticParams/getStaticPaths errors
The fix is correct, well-scoped, and matches the intended Next.js parity. I verified the full chain:
- Endpoint behavior confirmed (
server/app-prerender-endpoints.ts):notFoundResponse()→ 404 for disabled/stale-secret,jsonResponse({ error: String(error) }, 500)for a thrown user function. Branching onres.status === 500correctly distinguishes the two and preserves the legitimate warn-and-skip path for non-500 (404/403). - App Router throw propagation: the throw from the Proxy
gettrap (prerender.ts:1062) propagates up through thegenerateStaticParamsFncalls inside thetryblock and is caught by the existing per-route collector atprerender.ts:1269(Failed to call generateStaticParams(): …). Correct. - Pages Router: previously there was no try/catch around
getStaticPaths(only call site atprerender.ts:752), so a throw would crash the wholeprerenderPagesloop. The new try/catch converts it to a per-routeerror, consistent with the surroundingentryErrorpattern. Correct. parsePrerenderEndpointErrorfallback to raw text is reasonable for non-JSON 5xx (e.g. a crashing prod server).- Tests pass (3 new, 83 total),
vp checkis clean on both files.
One accuracy issue with the PR description (not the code) — see inline comment.
| // statuses (e.g. 404 = disabled/stale secret) keep the warn-and-skip | ||
| // behavior. Refs cloudflare/vinext#1982 | ||
| if (res.status === 500) { | ||
| throw new Error(parsePrerenderEndpointError(text)); |
There was a problem hiding this comment.
The code here is correct, but the PR description's claim that in default mode "the route fails with the real cause and the build fails" is not accurate.
In default (non-export) mode, a per-route status: "error" only increments the errors counter and is printed in the progress output — run-prerender.ts:343 only throws when mode === "export" && errors > 0. So the default build still completes successfully; this fix makes the error visible and accurately attributed (instead of being mis-reported as a "stale secret" skip), but it does not make the default-mode build fail.
Worth double-checking against Next.js: a throwing generateStaticParams does fail next build in default mode. If matching that is in scope for #1982, default mode would also need to throw on these error routes (or at least on generateStaticParams/getStaticPaths throws specifically, as opposed to e.g. ssr skips). If it's out of scope, consider tightening the PR description to say the error is now surfaced/reported rather than that "the build fails."
There was a problem hiding this comment.
Good catch — you're right that a per-route error alone didn't fail the default build (run-prerender.ts only threw under mode === "export"). Since #1982 explicitly wants parity with next build (which fails on a throwing generateStaticParams/getStaticPaths), I made it fail rather than just documenting it:
- The
generateStaticParams/getStaticPathserror results are now flaggedfatal. run-prerenderthrows for anyfatalroute in every mode, before the export-only check.- Intentionally-skipped dynamic/SSR routes stay non-
fatal, so default-mode leniency is preserved for them — only a genuine thrown user function fails the build.
PR description updated accordingly. Pushed in df06be99.
| fs.writeFileSync( | ||
| path.join(pageDir, "page.tsx"), | ||
| "export default function Page() { return null; }\n", | ||
| ); |
There was a problem hiding this comment.
Minor: because no prerender secret is set, the mock server short-circuits the request before the real app-prerender-endpoints.ts is involved (the stderr shows the prerender secret not found warning). That's fine for unit-testing the proxy's status branching, but these tests don't exercise the real endpoint's 500 path end-to-end. Worth a brief comment noting the endpoint response is mocked, so a future change to the real endpoint's status codes wouldn't be caught here.
There was a problem hiding this comment.
Added a note on the describe block clarifying that these tests mock the endpoint's HTTP response (the prod server here has no secret configured), so they exercise the build-side proxy's status branching rather than the real app-prerender-endpoints.ts 500 path end-to-end — and pointing at tests/app-prerender-endpoints.test.ts, which covers the endpoint's own throw→500 ({ error }) behaviour. Pushed in df06be99.
|
Review posted successfully: #2017 (review) SummaryI reviewed PR #2017 and verified the full chain of changes against the source. The fix is correct and well-scoped: Verified correct:
One issue raised (inline):
Minor (inline):
This was posted as a non-blocking |
bed6200 to
df06be9
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: surface thrown generateStaticParams/getStaticPaths errors + fail the build
I verified the full chain against the source and ran the relevant suites locally. The core fix is correct and well-scoped, and the follow-up that makes these errors fatal in all modes is a sound parity improvement. Details:
Verified correct
- Endpoint discriminator is right:
app-prerender-endpoints.tsreturnsnotFoundResponse()(404) for disabled/stale-secret andjsonResponse({ error: String(error) }, 500)only from thetry/catchwrapping the user function. Sores.status === 500genuinely means the user'sgenerateStaticParams/getStaticPathsthrew. Branching on it (and keeping warn-skip for non-500) is exactly the right cut. - App Router: the 500 throw at
prerender.ts:1070propagates up through the dynamic-route block and is caught by the existing per-route collector atprerender.ts:1285. Correct. - Pages Router: the new
try/catchatprerender.ts:758is narrowly scoped around just thegetStaticPathscall, converting a throw into a per-route error instead of crashing the loop. Good — and it correctly does not broaden the existing missing-paramsnormalization error (prerender.ts:802), which stays non-fatal, so themissing-paramstest still passes. vp checkclean;tests/prerender.test.ts(incl. export-mode) green locally. Export-mode still producesStatic export failed(its errors aren'tfatal), so the new fatal check atrun-prerender.ts:348doesn't shadow it.
Issues raised inline
- (test gap) No test exercises the actual
run-prerender.tsfatal-throw — the headline "fail the build in every mode" claim is unverified end-to-end. The new tests only assertprerenderApp/prerenderPagesreturn{ fatal: true }. - (behavior breadth) The App Router catch is broad: a transport/
fetchfailure insideresolveParentParams/generateStaticParamsFn(e.g. prod server crash, connection reset) is now flaggedfataland fails the build in all modes, even though it isn't a usergenerateStaticParamserror. Worth confirming this is intended. - (minor)
parsePrerenderEndpointErrorfalls back to the raw JSON body when{ error }is empty/missing, leaking{"error":""}into the message.
None are blocking; (1) is the one I'd most like addressed before merge.
| const route = result.routes.find((r) => r.route.includes("slug")); | ||
| // `fatal: true` makes run-prerender fail the build in default mode too, | ||
| // matching Next.js (not just a visible-but-non-fatal error). #1982 | ||
| expect(route).toMatchObject({ status: "error", fatal: true }); |
There was a problem hiding this comment.
Test gap: these tests assert that prerenderApp/prerenderPages return { status: "error", fatal: true }, but nothing exercises the actual build-failing behavior in run-prerender.ts:348 — i.e. that runPrerender now throws Prerender failed: ... in default mode. That throw is the headline change of this PR ("fail the build in every mode"), yet it's only covered transitively.
A cheap focused test would feed a synthetic fatal route through runPrerender and assert rejects.toThrow(/Prerender failed/) in mode: "default", mirroring the existing output: 'export' wiring block at line 1399. Without it, a regression that drops the fatal filter (or stops setting fatal) would still pass this suite.
There was a problem hiding this comment.
Good call — added direct coverage. Extracted the fatal-route gate into an exported assertNoFatalPrerenderRoutes(routes) (run-prerender.ts) that runPrerender now calls, and added two focused tests feeding it synthetic routes: a { status: "error", fatal: true } route → toThrow(/Prerender failed/), and a skipped SSR route + a non-fatal error → not.toThrow(). So a regression that drops the fatal filter (or stops setting fatal) now fails the suite. Pushed in ee4bcff2.
| route: route.pattern, | ||
| status: "error", | ||
| error: `Failed to call generateStaticParams(): ${detail}`, | ||
| fatal: true, |
There was a problem hiding this comment.
This catch wraps the entire dynamic-route block — resolveParentParams, every generateStaticParamsFn call, the "no params object" throw (line 1237), and buildUrlFromParams. Flagging all of them fatal: true means a transport/fetch failure (prod server crash, connection reset, ECONNREFUSED mid-prerender) — which throws a TypeError from fetch, not a 500 — now fails the build in every mode too, even though it isn't a thrown user generateStaticParams.
That may be acceptable (failing is arguably safer than silently shipping incomplete prerenders), but it's broader than "a thrown generateStaticParams is fatal" as described. If you want to scope fatal strictly to user-function throws, gate it on the 500 path (e.g. tag the error thrown at line 1070 and only mark fatal for that), so genuine infra errors stay non-fatal in default mode. At minimum worth a comment that any error in this block is now treated as fatal.
There was a problem hiding this comment.
Agreed — scoped fatal strictly to user-function throws. The 500 path now throws a dedicated PrerenderUserFunctionError (not a plain Error), and both catch sites only set fatal: true when e instanceof PrerenderUserFunctionError. So a transport/fetch failure (prod server crash, ECONNREFUSED) is still recorded as a per-route error but stays non-fatal — it only fails the build under output: 'export', exactly as before. Pushed in ee4bcff2.
| const parsed = JSON.parse(text) as { error?: unknown }; | ||
| if (parsed && typeof parsed.error === "string" && parsed.error.length > 0) { | ||
| return parsed.error; | ||
| } |
There was a problem hiding this comment.
Minor: when the 500 body is valid JSON but error is empty or non-string (e.g. {"error":""}), this falls through to return text, surfacing the raw {"error":""} JSON as the user-facing message. Consider returning the "Unknown prerender endpoint error" fallback in that case instead of echoing the JSON envelope.
There was a problem hiding this comment.
Fixed — parsePrerenderEndpointError now detects our JSON envelope ("error" in parsed) and returns the "Unknown prerender endpoint error" fallback when error is empty or non-string, instead of echoing the raw {"error":""}. A non-JSON body still falls back to the raw text. Pushed in ee4bcff2.
|
Review posted successfully: #2017 (review) SummaryI reviewed PR #2017, verified the full chain of changes against the source, and ran the relevant test suites locally ( Verified correct:
Issues raised inline (none blocking):
Posted as a non-blocking |
…rors The build-time prerender proxies that fetch the static-params (App Router) and pages-static-paths (Pages Router) endpoints treated every non-ok response identically: they logged a misleading "stale or missing prerender secret" warning and returned the no-params sentinel, discarding the response body. But the endpoint returns 404 for the genuine disabled/secret case and 500 (with the real error in its JSON body) when the user's generateStaticParams/getStaticPaths throws. Swallowing the 500 silently dropped the route from prerendering and let the default-mode build "succeed" while shipping an app missing pre-rendered pages, blaming a non-existent secret problem. Branch on status: a 500 throws the real error (parsed from the body). App Router's existing per-route collector turns it into a "Failed to call generateStaticParams(): ..." error; the Pages Router getStaticPaths call is now wrapped to produce the same per-route error instead of crashing. Non-500 responses keep the warn-and-skip path, so the legitimate secret/disabled case is unchanged. This matches Next.js, which fails the build with the real error. Fixes cloudflare#1982 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
df06be9 to
ee4bcff
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: surface thrown generateStaticParams/getStaticPaths errors
Re-reviewed the final state (commit ee4bcff2). The fix is correct and well-scoped, and all three concerns from prior reviews have been properly addressed:
- ✅ Fatal scoping:
fatal: trueis now gated strictly one instanceof PrerenderUserFunctionError(prerender.ts:777, 1300), so a transport/fetchfailure stays non-fatal and only fails underoutput: 'export'. Verified against the endpoint —app-prerender-endpoints.tsreturns 500 only from thetry/catcharound the user function (lines 72, 101), sores.status === 500is the right discriminator. The 400/404 paths are distinct and keep warn-and-skip. - ✅ Empty-error fallback:
parsePrerenderEndpointErrornow returns"Unknown prerender endpoint error"for an empty/non-stringerrorfield instead of echoing{"error":""}(prerender.ts:91-103). - ✅ Test gap:
assertNoFatalPrerenderRoutesis extracted and directly unit-tested for both the fatal-throw and the non-fatal/skip cases, so a regression that drops thefatalfilter would now fail the suite.
Verified locally: tests/prerender.test.ts -t "1982" green (5 pass), vp check clean on both touched files.
Two minor, non-blocking observations below.
| results.push({ | ||
| route: route.pattern, | ||
| status: "error", | ||
| error: `Failed to call getStaticPaths(): ${(e as Error).message}`, |
There was a problem hiding this comment.
Minor consistency: this catch uses (e as Error).message, but the other error-recording sites in this file gate on config.enablePrerenderSourceMaps to optionally include the stack via getErrorMessageWithStack — see prerender.ts:874 (SSG render) and prerender.ts:1291 (App Router generateStaticParams). config is in scope here too.
In practice the stack matters less here since the message is a stringified error coming back over HTTP from the endpoint (not a local throw with a useful stack), so this is purely a consistency nit, not a correctness issue.
| error: `Failed to call generateStaticParams(): ${detail}`, | ||
| // Only a thrown user generateStaticParams (a 500 from the endpoint) is | ||
| // fatal to the build; transport/fetch failures stay non-fatal. #1982 | ||
| ...(e instanceof PrerenderUserFunctionError ? { fatal: true as const } : {}), |
There was a problem hiding this comment.
Attribution note (not blocking): this catch also wraps resolveParentParams (prerender.ts:1191), which invokes a parent segment's generateStaticParamsFn at prerender.ts:522. If a parent route's generateStaticParams throws, it surfaces as a PrerenderUserFunctionError here and is attributed to the child route's pattern (Failed to call generateStaticParams(): … for the child), even though the throw originated in the parent.
The build still correctly fails (the error is a genuine user-function throw and is rightly fatal), so this is just a message-attribution quirk worth being aware of — not a behavioral bug.
|
Review posted successfully: #2017 (review) SummaryI re-reviewed PR #2017 at its final state (commit Verified correct:
Two minor, non-blocking observations flagged inline:
Posted as a non-blocking |
2 participants
Summary
generateStaticParams/getStaticPathsduring prerender instead of swallowing it. The build-time proxies that fetch the prerender static-params/static-paths endpoints now throw the genuine error message on an HTTP 500, so the route is recorded as a per-routeerrorwith the real cause (App Router via its existing collector; Pages Router via a new try/catch).output: 'export'. The per-route error is flaggedfatal, andrun-prerendernow throws for anyfatalroute regardless of mode — matchingnext build, which fails on a throwinggenerateStaticParams/getStaticPaths. Intentionally-skipped dynamic/SSR routes are unaffected (they are notfatal).500(which carries the user error in its JSON body) is treated as a real failure.parsePrerenderEndpointErrorhelper that extracts the{ error }message from the 500 body, falling back to the raw text.Root Cause
Both build-time proxies (
packages/vinext/src/build/prerender.ts) that fetch/__vinext/prerender/static-params(App Router) and/__vinext/prerender/pages-static-paths(Pages Router) treated any!res.okresponse identically: they logged"… This may indicate a stale or missing prerender secret."and returned the no-params sentinel (null/{ paths: [], fallback: false }), discarding the response body.But the endpoint (
packages/vinext/src/server/app-prerender-endpoints.ts) returns two very different non-ok responses:notFoundResponse()→ 404 for the genuine disabled / stale-secret case;jsonResponse({ error: String(error) }, 500)→ 500 when the user'sgenerateStaticParams/getStaticPathsthrows, with the real error in the body.Because the proxy swallowed the 500, a throwing
generateStaticParamswas mis-reported as "no static params", the route was silently dropped from prerendering, and the only console output blamed a non-existent secret problem — while the default-mode build still "succeeded" and shipped an app missing pre-rendered pages. Inoutput: 'export'it failed with the misleadingDynamic route requires generateStaticParams()instead of the user's error.The fix branches on status: a
500throwsparsePrerenderEndpointError(text). For App Router that throw is caught by the existing per-route collector (Failed to call generateStaticParams(): …); for Pages Router thegetStaticPathsconsumption is now wrapped to produce the same per-routeerror. Non-500 responses keep the warn-and-skip behavior, so the legitimate secret/disabled path is unchanged.This matches Next.js, which invokes the user function directly with no swallowing wrapper and fails the build with the real error/stack.
References
generateStaticParamserrors fail the build with the real message: https://github.com/vercel/next.js/blob/canary/test/production/app-dir/generate-static-params-errors/generate-static-params-errors.test.tspackages/next/src/build/static-paths/app.ts,packages/next/src/build/static-paths/pages.tsVerification
pnpm test tests/prerender.test.ts tests/run-prerender-concurrency.test.ts— green (3 new prerender cases: App Router throw flaggedfatal, Pages Router throw flaggedfatal, 404/secret still warn-skips)tests/app-prerender-endpoints.test.tspnpm test— full Vitest suite green (only pre-existing local-env flakes unrelated to this change)npx vp checkon the touched files — format, lint, types clean