fix(pages): route dotted dynamic paths in dev#2424
Conversation
|
/bigbonk review for issues |
commit: |
|
|
Working tree clean. Here's my review summary. Review of PR #2424 —
|
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 |
There was a problem hiding this comment.
Review of PR #2424 — fix(pages): route dotted dynamic paths in dev
I traced the dev request pipeline, verified parity with the real matchers, checked the fixtures, ran the new tests, and ran vp check.
Verdict: LGTM — no blocking issues
The fix is correct and well-scoped. The dev middleware previously short-circuited to next() for any path containing a . (except .html), incorrectly bouncing legitimate dotted dynamic Pages routes (e.g. /docs/release/v1.2, /api/users/alpha.beta) to Vite's static handler. The PR adds a preflight matchRoute against the page and API routers, keeping dotted requests that actually match a route in the Pages pipeline.
Notably, the second commit (7d35410) already closes the i18n gap that prior reviews of the first commit would have flagged: locale-prefixed dotted paths are now resolved through resolvePagesI18nRequest (pages) and stripI18nLocaleForApiRoute (api) before the preflight match.
What I verified
- Parity with the real matchers. The preflight's page-route match (index.ts:4164-4173) mirrors the downstream
matchPageRoute(index.ts:4321-4336) — both runresolvePagesI18nRequest(...).url. The preflight's API match viastripI18nLocaleForApiRoute(pathname, ...)(index.ts:4174) mirrors the pipeline'sstripI18nLocaleForApiRoute(resolvedUrl, ...)(pages-request-pipeline.ts:513). The two sides stay in sync. - No perf regression.
pagesRouter/apiRouterare cached by(pagesDir, extensions)(routeCachein routing/pages-router.ts:28), and the same routers are already invoked later (index.ts:4297, 4413). The addedPromise.allis a cache hit on the hot path, consistent with the AGENTS.md "reuse already-computed derivations" guidance. - Hybrid app/pages safe. For dotted paths that match no Pages route, the preflight still calls
next(), preserving the prior behavior so App Router RSC middleware can handle app-dir-only routes. No regression. - i18n redirect path intact. A non-prefixed dotted path that matches a dynamic route stays in the pipeline (preflight uses
pathnameunchanged when there's no prefix), so the locale redirect still fires. - Fixtures back the assertions.
pages/docs/[...slug].tsxemits theDocs/Path:markup andpages/api/users/[id].tsreturns{ user: { id, name } }exactly as asserted. - Tests + check pass. All 6 new tests (dev dotted page/api, i18n locale-prefixed dotted page/api, and the 2 prod regression tests) pass.
vp check packages/vinext/src/index.ts tests/pages-router.test.tsis clean (format, lint, types). - Prod parity.
prod-server.tshad no dot-skip preflight (this was a dev-only gap), and the two new prod tests confirm prod already handled these correctly — good defensive coverage. - Tests cite the relevant Next.js sources (
base-server.ts#getRouteMatchers,router-utils/filesystem.ts), matching the AGENTS.md parity-anchoring requirement.
Non-blocking observations
-
Asymmetric i18n resolution between page and API preflight (intentional, but worth a note).
pageRouteUrlgoes throughresolvePagesI18nRequestwhileapiRouteUrlgoes throughstripI18nLocaleForApiRoute. This is correct — each mirrors its respective downstream path — but a one-line comment explaining why the two differ would help future readers avoid "fixing" the asymmetry. -
requestHostnameparsing (nit). The host split at index.ts:4160-4163 handles array/undefinedhost headers and strips the port. This duplicates thenew URL(request.url).hostnamederivation used bymatchPageRoute(index.ts:4327). Both produce a bare hostname, so behavior is equivalent; consolidating the two into a shared helper would reduce drift risk if hostname handling changes later.
Neither blocks merge.
|
Review posted on PR #2424. SummaryVerdict: LGTM — no blocking issues PR #2424 fixes a dev-only bug where the Pages Router dev middleware short-circuited to What I verified:
Two non-blocking nits: a comment explaining the intentional page-vs-API i18n resolution asymmetry, and consolidating the duplicated hostname derivation. |
|
/bigbonk review for issues |
Review of PR #2424 —
|
|
Posted my review to PR #2424. Summary: Verdict: LGTM — no blocking issuesThe fix is correct and well-scoped. I focused on the one thing prior reviews hadn't fully chased down: whether keeping dotted paths in the pipeline could regress public-file precedence. Key findings:
Two non-blocking nits (the intentional page-vs-API i18n asymmetry deserving its comment, and a duplicated hostname derivation that could be a shared helper). Neither blocks merge. |
|
/bigbonk review for issues |
Review of PR #2424 —
|
|
Posted my review to PR #2424. Verdict: LGTM — no blocking issuesThe fix is correct and well-scoped. I reviewed all 4 commits (noting Key findings (things I independently chased down):
Two non-blocking nits: a pre-existing |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review of PR #2424 — fix(pages): route dotted dynamic paths in dev
I reviewed all 5 commits independently: read the full index.ts diff, traced the dev request pipeline, verified parity against the downstream matchers and the shared Pages pipeline, checked the fixtures, ran the new tests, and ran vp check.
Verdict: LGTM — no blocking issues
The fix is correct and well-scoped. The dev Pages middleware previously short-circuited to next() for any path containing a . (except .html), incorrectly bouncing legitimate dotted dynamic routes (/docs/release/v1.2, /api/users/alpha.beta) to Vite's static handler. The PR adds a preflight matchRoute against the page and API routers (index.ts:4161-4182) so dotted requests that actually match a route stay in the Pages pipeline.
What I verified
- Same route source — no drift. The preflight's
pageRoutes/apiRoutescome from the identical cachedpagesRouter(pagesDir, nextConfig?.pageExtensions, fileMatcher)/apiRouter(...)calls used later fordevPageRoutes(index.ts:4299) and the API lookup (index.ts:4415). The two sides cannot disagree. - Page parity. Preflight runs
resolvePagesI18nRequest(...).url(index.ts:4168-4177), mirroring the downstreammatchPageRoute(index.ts:4323-4338). Confirmed againstresolvePagesI18nRequestinpages-i18n.ts— for a non-prefixed/api/...pathextractLocaleFromUrlreturns the URL unchanged, so the page branch correctly fails to match and the API branch handles it. - API parity. Preflight runs
stripI18nLocaleForApiRoute(pathname, ...)(index.ts:4178), mirroring the pipeline'sstripI18nLocaleForApiRoute(resolvedUrl, ...)(pages-request-pipeline.ts:513). The preflight strips from the pre-middlewarepathnamewhile the pipeline strips from the post-middlewareresolvedUrl; for this gate (stay-in-pipeline vsnext()) the permissive direction is safe — the pipeline re-resolves authoritatively. - No public-file regression. The Pages middleware is registered inside the
configureServerpost-hook (return () => { server.middlewares.use(...) }, index.ts:3711/4542), which runs after Vite's internal public/static middlewares. Realpublic/files are served before reaching the preflight, so only non-file dotted paths get here. - Hybrid app+pages safe. Dotted paths matching no Pages route still
next()(index.ts:4183-4185), preserving App Router handling. App-only apps never reach the preflight (gated byhasPagesDir). - No perf regression. Routers are cached by
(pagesDir, extensions)viarouteCache, so the addedPromise.allis a hot-path cache hit, and it only fires for dotted, non-rewrite requests. - Hostname dedup is a genuine improvement.
requestHostnow picksreq.headers.host[0]for array headers and is reused forrequestOrigin,requestHostname, andmwOrigin. The oldhttp://${req.headers.host || "localhost"}would have joined an array host intohttp://a,b.getUrlHostname(requestOrigin)correctly strips any port, matching the downstreamnew URL(request.url).hostname. - Prior nit addressed. The dev image passthrough now uses the shared
requestOrigin(index.ts:3995) instead of the old inlinehttp://${req.headers.host || "localhost"}pattern flagged in earlier reviews. - Tests + check pass. All 9 matching tests pass (dev dotted page/api, i18n locale-prefixed dotted page/api, 2 prod regression tests);
vp check packages/vinext/src/index.ts tests/pages-router.test.tsis clean (format, lint, types). Tests anchor parity to the relevant Next.js sources.
Non-blocking observations
- Page-vs-API i18n asymmetry is intentional and documented. The comment at index.ts:4166-4167 explains why the page side uses full i18n resolution while the API side only strips a locale prefix. Keep it — it's the thing that prevents a future reader from "fixing" the asymmetry.
No blockers. Ship it.
|
Review posted to PR #2424. Verdict: LGTM — no blocking issuesThe fix is correct and well-scoped. The dev Pages middleware previously short-circuited to What I independently verified:
One non-blocking note: the page-vs-API i18n asymmetry is intentional and already has its explanatory comment — keep it. |
Summary
Validation
Review