Repository navigation
fix(pages): match optional catch-all prerender paths - #2238
Conversation
|
/bigbonk review for issues |
commit: |
|
There was a problem hiding this comment.
Review: fix(pages): match optional catch-all prerender paths
I verified the change against the authoritative Next.js source (.nextjs-ref/packages/next/src/build/static-paths/pages.ts, v16 canary) and ran the focused suites. The implementation is correct and faithfully mirrors Next.js. No blocking issues.
What I checked
Parity with Next.js buildPagesStaticPaths — the new matchesPagesStaticPath reproduces the upstream per-param validation loop closely:
- Iterates
routeParameterKeys(heregetPagesRouteParamsderived from the bracket pattern), not the raw entry keys — matches Next.js, so extra entry keys are correctly ignored. - Optional catch-all normalization of
null | undefined | false→[]matches Next.js (paramValue = []). - Required catch-all with a non-array (or empty) value is rejected — matches the Next.js
repeat && !Array.isArray(paramValue)throw. The "rejects required catch-all"it.eachcovers all four cases. - Single dynamic params require
typeof value === "string"— matches Next.js. (vinext returns 404 instead of throwing at build time; this is the pre-existing, documented behavioral choice noted in the JSDoc, and is fine.)
Object.hasOwn gate — Next.js only normalizes optional [] when params.hasOwnProperty(key) is true; an entirely-omitted key falls through to the typeof paramValue === 'undefined' throw. The PR's !Object.hasOwn(entryParams, key) → return false correctly reproduces this (404 instead of throw).
Runtime param shape — confirmed via route-trie.ts:163-169 that an empty optional catch-all does not materialize the param (params.slug is undefined, not []). The optional && value.length === 0 && actual === undefined → true branch is exactly what handles this, and the empty-array fallthrough also resolves correctly for the non-empty cases.
Dev/prod parity — both paths feed the same Next.js bracket pattern into getPagesRouteParams (patternToNextFormat(route.pattern) in dev-server.ts and pages-page-handler.ts:418; options.routePattern in resolvePagesPageData). Sharing the matcher removes the prior dev-only divergence. No dead imports left behind in dev-server.ts.
Regex ordering in getPagesRouteParams — optional [[...x]] is matched before required [...x] before dynamic [x], so greedier patterns can't shadow more specific ones. Correct.
Verification run
tests/pages-page-data.test.ts— 53/53 passtests/pages-router.test.ts -t "optional catch-all"+ mixed-catchall dev tests — pass (/catchall-optional→ 200,/mixed-catchall/guides→ 200,/mixed-catchall/unlisted→ 404)vp checkon both changed source files — clean (format, lint, types)
Minor nit (non-blocking)
In pages-page-data.ts, the JSDoc block describing matchesPagesStaticPath (lines 443-457) now sits directly above the newly-inserted PagesRouteParam type and getPagesRouteParams function, leaving matchesPagesStaticPath itself (line 485) undocumented. Consider moving that JSDoc back down to matchesPagesStaticPath and giving getPagesRouteParams its own short comment.
LGTM.
|
Review posted to PR #2238. SummaryI reviewed the change against the authoritative Next.js source and ran the focused test suites. The implementation is correct — no blocking issues. Key findings (all verified clean):
Verification: 53/53 page-data unit tests pass, dev integration tests pass (root → 200, mixed → 200, unlisted → 404), One minor non-blocking nit: the JSDoc for Overall: LGTM. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Summary
getStaticPathsempty values like Next.jsValidation
vp checkNext.js parity
Targets the optional catch-all prerender failure from
test/e2e/prerender.test.tsin deploy-suite run 27858251903. Verified against Next.js v16.2.6 static-path normalization. No raw Next.js E2E was run locally.