fix(app-router): honor basePath false rewrites#2361
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): honor basePath false rewrites
Reviewed the diff against Next.js v16.2.6 behavior. The approach is sound and well-targeted. Verified locally:
tsc --noEmitclean for the vinext packagetests/app-rsc-request-normalization.test.ts+tests/app-rsc-handler.test.ts— 148 passedtests/app-router-external-rewrite.test.ts+tests/app-router-next-config-dev.test.ts— 21 passed- Added an ad-hoc test confirming the afterFiles
basePath: falseshape (the actual Next.js fixture shape) also works out-of-basePath — passes.
What looks correct
normalizeRscRequest's newoptions.allowOutOfBasePathdefaults to{}, so every existing caller (and the 50+ tests that call it without options) keeps the strict 404 behavior. Only the App Router handler opts in. Good backward-compat.canUseFilesystemRoutes()as a re-evaluated closure overdidMiddlewareRewrite/configRewriteFiredis the right design — later checks correctly observe the updated state after middleware/rewrites flip the flags.- The
redirectDestinationWithBasePath(..., hadBasePath)gate is correct: with the newbasePathState,matchRedirectonly matchesbasePath: truerules for in-basePath requests, so out-of-basePath redirects are only everbasePath: falserules and must not be re-prefixed. - The removed
TODO(#1333)and stalebasePathState = { ..., hadBasePath: true }comment are appropriately deleted now that the issue is resolved. stripBasePathis a no-op for non-prefixed paths, so the refactored normalization branch (else if (hadBasePath)) preserves the old/__vinext/behavior; settinghadBasePath = truefor/__vinext/paths is correct since they are internal endpoints.
Non-blocking observations
-
Test coverage gap vs. the real fixture. The Next.js fixture (
test/e2e/app-dir/app-basepath/next.config.js) returns a flat array fromrewrites(), which Next.js maps toafterFiles— but the added unit test only exercisesbeforeFiles. The afterFiles path does work (I verified), but since that's the exact shape the cited Next.js test relies on, consider adding anafterFiles(and ideallyfallback) variant of the test so a future regression in the post-match rewrite loops is caught. The two loops setconfigRewriteFired = trueindependently from the beforeFiles loop. -
Pages/App parity divergence (pre-existing, worth tracking).
pages-request-pipeline.ts:502rejects out-of-basePath requests with a 404 before the afterFiles/fallback loops run (gated only on!configRewriteFired, which beforeFiles sets). This PR makes App Router honorbasePath: falserewrites in afterFiles/fallback too, which is the more Next.js-correct behavior — but it means abasePath: falseafterFiles/fallback rewrite now behaves differently between Pages Router and App Router. Per AGENTS.md's dev/prod + Pages/App parity guidance, this is worth a follow-up issue to bring the Pages pipeline in line (or a deliberate, documented note on why they differ).
Neither observation blocks merge. LGTM.
|
Review posted to PR #2361. SummaryThe PR is well-scoped and correct. It resolves issue #1333 by letting out-of-basePath App Router requests reach config rules that explicitly opt out via Verified:
Two non-blocking observations raised in the review:
No blockers — 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
basePathto reach config rules that explicitly opt out withbasePath: falseNext.js parity
Fixes the remaining failure in:
test/e2e/app-dir/app-basepath/index.test.tsshould redirect externally when encountering absolute URLs on the same host outside the basePathReference: https://github.com/vercel/next.js/blob/v16.2.6/test/e2e/app-dir/app-basepath/index.test.ts
Validation
vp test run tests/app-rsc-request-normalization.test.ts tests/app-rsc-handler.test.ts --maxWorkers=1— 148 passedvp checkfor all four changed files