fix catch-all route normalization for default parallel routes #60240

ztanner · 2024-01-04T21:05:11Z

What?

When relying on a default route as a fallback, greedier catch-all segments in the application hierarchy would take precedence, causing unexpected errors/matching behavior.

Why?

When performing parallel route catch-all normalization, we push potential catch-all matches to paths without considering that a path might instead be matched by a default page. Because of this, the catch-all take precedence and the app will not try and load the default.

For example, given this structure:

{
  "/": ["/page"],
  "/[[...catchAll]]": ["/[[...catchAll]]/page"],
  "/nested/[foo]/[bar]": ["/nested/[foo]/[bar]/@slot/page"],
  "/nested/[foo]/[bar]/[baz]": ["/nested/[foo]/[bar]/@slot/[baz]/page"],
}

(Where there's a /nested/[foo]/[bar]/default.tsx)

The route normalization logic would produce:

{
  "/": ["/page"],
  "/[[...catchAll]]": ["/[[...catchAll]]/page"],
  "/nested/[foo]/[bar]": [
    "/nested/[foo]/[bar]/@slot/page",
    "/[[...catchAll]]/page",
  ],
  "/nested/[foo]/[bar]/[baz]": [
    "/nested/[foo]/[bar]/@slot/[baz]/page",
    "/[[...catchAll]]/page",
  ],
}

This means that when building the LoaderTree, it won't ever try to find the default for that segment. This solution operates under the assumption that if you defined a default at a particular layout segment, you intend for that to render in place of a greedier catch-all. (Let me know if this is an incorrect assumption)

How?

We can't safely normalize catch-all parallel routes without having context about where the default segments are, so this updates appPaths to be inclusive of default segments and then filters them when doing anything relating to build/export to maintain existing behavior. We use this information to check if an existing default exists at the same segment level that we'd push the catch-all to. If one exists, we don't push the catch-all. Otherwise we proceed as normal.

Closes NEXT-1987

ztanner · 2024-01-04T21:05:23Z

Current dependencies on/for this PR:

canary
- PR fix catch-all route normalization for default parallel routes #60240 👈

This stack of pull requests is managed by Graphite.

ijjk · 2024-01-04T21:10:30Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
buildDuration	12.9s	12.9s	N/A
buildDurationCached	7.3s	6.3s	N/A
nodeModulesSize	200 MB	200 MB	⚠️ +6.45 kB
nextStartRea..uration (ms)	410ms	407ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
193.HASH.js gzip	181 B	182 B	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	✓
433-HASH.js gzip	28.6 kB	28.6 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	239 B	242 B	N/A
main-HASH.js gzip	31.7 kB	31.8 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	N/A
Overall change	98.5 kB	98.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
_app-HASH.js gzip	194 B	195 B	N/A
_error-HASH.js gzip	183 B	181 B	N/A
amp-HASH.js gzip	504 B	502 B	N/A
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	255 B	253 B	N/A
head-HASH.js gzip	350 B	349 B	N/A
hooks-HASH.js gzip	369 B	369 B	✓
image-HASH.js gzip	4.28 kB	4.28 kB	N/A
index-HASH.js gzip	255 B	256 B	N/A
link-HASH.js gzip	2.61 kB	2.61 kB	✓
routerDirect..HASH.js gzip	312 B	311 B	N/A
script-HASH.js gzip	385 B	383 B	N/A
withRouter-HASH.js gzip	307 B	308 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	3.4 kB	3.4 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
_buildManifest.js gzip	483 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
index.html gzip	527 B	529 B	N/A
link.html gzip	540 B	541 B	N/A
withRouter.html gzip	523 B	524 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
edge-ssr.js gzip	93.8 kB	93.8 kB	N/A
page.js gzip	147 kB	147 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
middleware-b..fest.js gzip	626 B	625 B	N/A
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	37.4 kB	37.4 kB	N/A
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	2.07 kB	2.07 kB	✓

Next Runtimes

	vercel/next.js canary	vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments	Change
app-page-exp...dev.js gzip	169 kB	169 kB	✓
app-page-exp..prod.js gzip	94.2 kB	94.2 kB	✓
app-page-tur..prod.js gzip	94.9 kB	94.9 kB	✓
app-page-tur..prod.js gzip	89.5 kB	89.5 kB	✓
app-page.run...dev.js gzip	139 kB	139 kB	✓
app-page.run..prod.js gzip	88.8 kB	88.8 kB	✓
app-route-ex...dev.js gzip	24.1 kB	24.1 kB	✓
app-route-ex..prod.js gzip	16.7 kB	16.7 kB	✓
app-route-tu..prod.js gzip	16.7 kB	16.7 kB	✓
app-route-tu..prod.js gzip	16.3 kB	16.3 kB	✓
app-route.ru...dev.js gzip	23.5 kB	23.5 kB	✓
app-route.ru..prod.js gzip	16.3 kB	16.3 kB	✓
pages-api-tu..prod.js gzip	9.38 kB	9.38 kB	✓
pages-api.ru...dev.js gzip	9.65 kB	9.65 kB	✓
pages-api.ru..prod.js gzip	9.37 kB	9.37 kB	✓
pages-turbo...prod.js gzip	21.9 kB	21.9 kB	✓
pages.runtim...dev.js gzip	22.6 kB	22.6 kB	✓
pages.runtim..prod.js gzip	21.9 kB	21.9 kB	✓
server.runti..prod.js gzip	49.5 kB	49.5 kB	✓
Overall change	932 kB	932 kB	✓

Commit: 83fb9eb

ijjk · 2024-01-04T21:13:09Z

Tests Passed

…greedier catch-all segments

…#60240) ### What? When relying on a `default` route as a fallback, greedier catch-all segments in the application hierarchy would take precedence, causing unexpected errors/matching behavior. ### Why? When performing parallel route catch-all normalization, we push potential catch-all matches to paths without considering that a path might instead be matched by a `default` page. Because of this, the catch-all take precedence and the app will not try and load the default. For example, given this structure: ``` { "/": ["/page"], "/[[...catchAll]]": ["/[[...catchAll]]/page"], "/nested/[foo]/[bar]": ["/nested/[foo]/[bar]/@slot/page"], "/nested/[foo]/[bar]/[baz]": ["/nested/[foo]/[bar]/@slot/[baz]/page"], } ``` (Where there's a `/nested/[foo]/[bar]/default.tsx`) The route normalization logic would produce: ``` { "/": ["/page"], "/[[...catchAll]]": ["/[[...catchAll]]/page"], "/nested/[foo]/[bar]": [ "/nested/[foo]/[bar]/@slot/page", "/[[...catchAll]]/page", ], "/nested/[foo]/[bar]/[baz]": [ "/nested/[foo]/[bar]/@slot/[baz]/page", "/[[...catchAll]]/page", ], } ``` This means that when building the `LoaderTree`, it won't ever try to find the default for that segment. **This solution operates under the assumption that if you defined a `default` at a particular layout segment, you intend for that to render in place of a greedier catch-all.** (Let me know if this is an incorrect assumption) ### How? We can't safely normalize catch-all parallel routes without having context about where the `default` segments are, so this updates `appPaths` to be inclusive of default segments and then filters them when doing anything relating to build/export to maintain existing behavior. We use this information to check if an existing default exists at the same segment level that we'd push the catch-all to. If one exists, we don't push the catch-all. Otherwise we proceed as normal. Closes NEXT-1987

### What `/default` segments were considered valid page outputs to handle catch-all route normalization (see #60240) but they shouldn't leak into the prerender manifest. This filters them out at build time. Closes NEXT-2053

… nested explicit (non-catchall) slot routes (#60776) Fix NEXT-2165 ### What? Addresses the limitation of #60240, where a dummy `default` file is required in parallel route child slot to prevent errors in dev server rendering (`TypeError: Cannot read properties of undefined (reading 'clientModules')`) as well as errors in build and deploy (`Error: ENOENT: no such file or directory, lstat ‘/vercel/path0/.next/server/app/parallel-route/[section]/@part/[partSlug]/page_client-reference-manifest.js’`) Without the `default.tsx`, builds and deployments will fail with: <img width="956" alt="CleanShot 2024-01-18 at 02 12 36@2x" src="https://github.com/vercel/next.js/assets/179761/80ba61bd-6ec0-4b16-a393-dc9375227e19"> local dev server will also crash with: <img width="986" alt="CleanShot 2024-01-18 at 02 13 19@2x" src="https://github.com/vercel/next.js/assets/179761/cc500a32-b2f8-47b4-999e-e57cf5141b2f"> > TypeError: Cannot read properties of undefined (reading 'clientModules') ### Why? Since `default.tsx` is not a compulsory when you have slot that are specific and ends with a dynamic route segment, this PR extends support so that it is possible mixing catch-all routes with specific non-catchall routes without requiring an additional `default.tsx` . This PR will allow the following test cases to pass: ``` it('should not add the catch-all route to segments that have a more specific [dynamicRoute]', () => { const appPaths = { '/': ['/page'], '/[[...catchAll]]': ['/[[...catchAll]]/page'], '/nested/[foo]/[bar]/default': [ '/nested/[foo]/[bar]/default', '/nested/[foo]/[bar]/@slot0/default', '/nested/[foo]/[bar]/@slot2/default', ], '/nested/[foo]/[bar]': [ '/nested/[foo]/[bar]/@slot0/page', '/nested/[foo]/[bar]/@slot1/page', ], '/nested/[foo]/[bar]/[baz]': [ '/nested/[foo]/[bar]/@slot0/[baz]/page', '/nested/[foo]/[bar]/@slot1/[baz]/page', ], '/[locale]/nested/[foo]/[bar]/[baz]/[qux]': [ '/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page', ], } const initialAppPaths = JSON.parse(JSON.stringify(appPaths)) normalizeCatchAllRoutes(appPaths) expect(appPaths).toMatchObject(initialAppPaths) }) ... ``` ```it('should not add the catch-all route to segments that have a more specific [dynamicRoute]', () => { const appPaths = { '/': ['/page'], '/[[...catchAll]]': ['/[[...catchAll]]/page'], '/nested/[foo]/[bar]/default': [ '/nested/[foo]/[bar]/default', '/nested/[foo]/[bar]/@slot0/default', '/nested/[foo]/[bar]/@slot2/default', ], '/nested/[foo]/[bar]': [ '/nested/[foo]/[bar]/@slot0/page', '/nested/[foo]/[bar]/@slot1/page', ], '/nested/[foo]/[bar]/[baz]': [ '/nested/[foo]/[bar]/@slot0/[baz]/page', '/nested/[foo]/[bar]/@slot1/[baz]/page', ], '/[locale]/nested/[foo]/[bar]/[baz]/[qux]': [ '/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page', ], } ... ``` and allow parallel routes defined in this [code repro](https://github.com/williamli/nextjs-NEXT-2165) to build. ![image](https://github.com/vercel/next.js/assets/179761/030f4fe1-3a27-41e5-bbd9-bc511f95e5d7) ### How? `packages/next/src/build/normalize-catchall-routes.ts` is extended to check `appPath` to see if it is: 1. the route is not a catchall 2. `isMoreSpecific` than the closest `catchAllRoute`. where `isMoreSpecific` is defined as: ``` function isMoreSpecific(pathname: string, catchAllRoute: string): boolean { const pathnameDepth = pathname.split('/').length const catchAllRouteDepth = catchAllRoute.split('/').length - 1 return pathnameDepth > catchAllRouteDepth } ``` --------- Co-authored-by: Zack Tanner <zacktanner@gmail.com>

Reverts changes from #61173 & #60240 (while leaving the tests that were added). There are too many spots where considering `/default` routes as pages needs to be carefully considered in different runtimes, and it turns out that it's not actually needed to handle the case that it was originally added for. I confirmed that the test that added the case it was intended to fix (`parallel-routes-catchall-default`, along with the unit tests in `normalize-catchall-routes`) are still passing as expected.

ijjk added area: tests created-by: Next.js team PRs by the Next.js team. type: next labels Jan 4, 2024

ztanner force-pushed the 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments branch from df109cf to b321561 Compare January 4, 2024 21:14

ztanner changed the title ~~fix catch-all route normalization for default routes~~ fix catch-all route normalization for default parallle routes Jan 4, 2024

ztanner changed the title ~~fix catch-all route normalization for default parallle routes~~ fix catch-all route normalization for default parallel routes Jan 4, 2024

ztanner changed the title ~~fix catch-all route normalization for default parallel routes~~ (wip) fix catch-all route normalization for default parallel routes Jan 5, 2024

ztanner force-pushed the 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments branch from b321561 to 73f0cdb Compare January 5, 2024 01:02

ensure more specific default segments match parallel routes before …

83fb9eb

…greedier catch-all segments

ztanner force-pushed the 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments branch from 73f0cdb to 83fb9eb Compare January 5, 2024 01:05

ztanner changed the title ~~(wip) fix catch-all route normalization for default parallel routes~~ fix catch-all route normalization for default parallel routes Jan 5, 2024

ztanner marked this pull request as ready for review January 5, 2024 14:38

ztanner requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, a team and wyattjoh as code owners January 5, 2024 14:38

feedthejim approved these changes Jan 5, 2024

View reviewed changes

ijjk approved these changes Jan 5, 2024

View reviewed changes

ztanner merged commit 90f9539 into canary Jan 5, 2024
75 checks passed

ztanner deleted the 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments branch January 5, 2024 22:20

samcx mentioned this pull request Jan 6, 2024

chore: update turbo to the latest #60293

Closed

ztanner mentioned this pull request Jan 11, 2024

filter default segments from prerender manifest #60499

Merged

williamli mentioned this pull request Jan 17, 2024

fix parallel route top-level catch-all normalization logic to support nested explicit (non-catchall) slot routes #60776

Merged

ztanner mentioned this pull request Jan 27, 2024

revert changes to process default routes at build #61241

Merged

github-actions bot added the locked label Feb 1, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024

vercel deleted a comment from williamli Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix catch-all route normalization for default parallel routes #60240

fix catch-all route normalization for default parallel routes #60240

ztanner commented Jan 4, 2024 •

edited

Loading

ztanner commented Jan 4, 2024

ijjk commented Jan 4, 2024 •

edited

Loading

ijjk commented Jan 4, 2024 •

edited

Loading

fix catch-all route normalization for default parallel routes #60240

fix catch-all route normalization for default parallel routes #60240

Conversation

ztanner commented Jan 4, 2024 • edited Loading

What?

Why?

How?

ztanner commented Jan 4, 2024

ijjk commented Jan 4, 2024 • edited Loading

Stats from current PR

ijjk commented Jan 4, 2024 • edited Loading

Tests Passed

ztanner commented Jan 4, 2024 •

edited

Loading

ijjk commented Jan 4, 2024 •

edited

Loading

ijjk commented Jan 4, 2024 •

edited

Loading