fix(app-router): let concrete Pages routes win middleware rewrites - #2730
Open
NathanDrake2406 wants to merge 1 commit into
Open
fix(app-router): let concrete Pages routes win middleware rewrites#2730NathanDrake2406 wants to merge 1 commit into
NathanDrake2406 wants to merge 1 commit into
Conversation
A Pages data request that middleware rewrote returned a synthetic empty JSON body whenever any App route matched the rewrite target, including a dynamic or catch-all match. Every other App-vs-Pages ownership decision in this handler treats a dynamic App match as non-owning, so a concrete Pages route at the same pathname should render instead. Skipping that arbitration meant getServerSideProps never ran for the rewrite target, and the client router, which reuses the middleware probe response when the rewrite target resolves to a Pages route, accepted the empty body as successful page data. Redirect and notFound markers the Pages route would have returned were therefore absent during client-side navigation. Restrict the shortcut to App matches that own the target outright, so dynamic matches fall through to the existing static and dynamic Pages fallback arbitration. That fallthrough reaches the tail Pages data response, which built its headers from the not-found response alone and dropped headers the middleware set on the way. Merge the middleware response headers there so a rewrite landing on a genuinely App-owned dynamic route still carries its cookies.
commit: |
Contributor
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Pages-data middleware rewrite shortcut added in #2468 treats any App route match as owning the rewrite target, including dynamic and catch-all matches. A concrete Pages route outranks a dynamic App match everywhere else in this handler, so the shortcut returned a synthetic
{}beforerenderPagesFallbackcould rungetServerSidePropsfor the target.Why
handleAppRscRequestencodes App-vs-Pages ownership asmatch === null || match.route.isDynamicin the two places that arbitrate it:renderPagesForMatchKind(app-rsc-handler.ts:989) and the afterFiles rewrite guard (:1018). A dynamic App match does not own a pathname, because a concrete Pages route is more specific.The fast path at
:902testedpreActionMatchfor truthiness alone, so a catch-all such asapp/[...slug]was treated as the owner. That matters beyond the response body, because the Pages client reuses the response:x-nextjs-rewriteas aMiddlewareDataEffect(shims/router.ts:1826).navigateClientDataasprefetchedResponse(:2555).navigateClientDataskips the real data fetch when given one (:2126), and{}becomesprops = {}with no__N_REDIRECTmarker (:2205).So a client-side navigation whose middleware rewrite lands on a Pages route shadowed by a dynamic App route rendered the page with empty props, without the redirect or
notFoundresult itsgetServerSidePropswould have returned. Document requests were unaffected, since they never take this path.Client-side ownership already agrees with the server rule:
resolveMatchedHybridClientRouteOwnerrunscompareHybridRoutePatterns, which gives the concrete Pages pattern precedence. Only the server shortcut disagreed.What changed
{}+x-nextjs-rewrite, middleware headers merged{}+x-nextjs-rewrite,getServerSidePropsskippedx-nextjs-rewrite{}+x-nextjs-rewrite, middleware headers mergedThe second hunk is required by the first. Restricting the shortcut moves dynamic-match requests onto the tail Pages-data response, which built its headers from
buildNextDataNotFoundResponse()alone and dropped whatever the middleware set. Without merging them there, gating the shortcut would trade this bug for a droppedSet-Cookieon every rewrite landing on a genuinely App-owned dynamic route.Maintainer review path
packages/vinext/src/server/app-rsc-handler.ts:902for the ownership predicate, which now matches:989and:1018.packages/vinext/src/server/app-rsc-handler.ts:1138for the header merge the fallthrough depends on.tests/app-rsc-handler.test.tsfor the two regression tests.Validation
getServerSidePropsredirect marker rather than{}.Set-Cookieand custom headers survive a rewrite that lands on a dynamic App route with no Pages route.expected {} to deeply equal { Object (pageProps) }; removing the merge fails withexpected null to be 'probe=1; Path=/'.tests/e2e/app-router/pages-to-app-routing.spec.tspasses in full, including the two cases fix(app-router): expose app rewrites to pages data clients #2468 added for Pages-to-App navigation through a middleware rewrite.Commands
Risk / compatibility
References
packages/vinext/src/server/app-rsc-handler.ts:989