fix parallel catch-all route normalization #59791

ztanner · 2023-12-20T04:11:58Z

What?

Catch-all routes are being matched to parallel routes which causes issues like an interception route being handled by the wrong page component, or a page component being associated with multiple pages resulting in a "You cannot have two parallel pages that resolve to the same path" build error.

Why?

#58215 fixed a bug that caused catchall paths to not properly match when used with parallel routes. In other words, a catchall slot wouldn't render on a page that could match that catch all. Or a catchall page wouldn't match a slot. At build time, a normalization step was introduced to take application paths and attempt to perform this matching behavior.

However in it's current form, this causes the errors mentioned above to manifest. To better illustrate the problem, here are a few examples:

Given:

{
  '/': [ '/page' ],
  '/[...slug]': [ '/[...slug]/page' ],
  '/items/[...ids]': [ '/items/[...ids]/page' ],
  '/(.)items/[...ids]': [ '/@modal/(.)items/[...ids]/page' ]
}

The normalization logic would produce:

{
  '/': [ '/page' ],
  '/[...slug]': [ '/[...slug]/page' ],
  '/items/[...ids]': [ '/items/[...ids]/page' ],
  '/(.)items/[...ids]': [ '/@modal/(.)items/[...ids]/page', '/[...slug]/page' ]
}

The interception route will now be improperly handled by [...slug]/page rather than the interception handler.

Another example, which rather than incorrectly handling a match, will produce a build error:

Given:

{
  '/': [ '/(group-b)/page' ],
  '/[...catcher]': [ '/(group-a)/@parallel/[...catcher]/page' ]
}

The normalization logic would produce:

{
  '/': [ '/(group-b)/page', '/(group-a)/@parallel/[...catcher]/page' ],
  '/[...catcher]': [ '/(group-a)/@parallel/[...catcher]/page' ]
}

The parallel catch-all slot is now part of /. This means when building the loader tree, two children parallel segments (aka page components) will be found when hitting /, which is an error.

The error that was added here was originally intended to help catch scenarios like:
/app/(group-a)/page and /app/(group-b)/page. However it also throws for parallel slots, which isn't necessarily an error (especially since the normalization logic will push potential matches).

How?

There are two small fixes in this PR, the rest are an abundance of e2e tests to help prevent regressions.

When normalizing catch-all routes, we will not attempt to push any page entrypoints for interception routes. These should already have all the information they need in appPaths.
Before throwing the error about duplicate page segments in next-app-loader, we check to see if it's because we already matched a page component but we also detected a parallel slot that would have matched the page slot. In this case, we don't error, since the app can recover from this.
Loading a client reference manifest shouldn't throw a cryptic require error. loadClientReferenceManifest is already potentially returning undefined, so this case should already be handled gracefully

Separately, we'll need to follow-up on the Turbopack side to:

Make sure the duplicate matching matches the Webpack implementation (I believe Webpack is sorting, but Turbopack isn't)
Implement parallel routes: fix catch all route support #58215 in Turbopack. Once this is done, we should expect the tests added in this PR to start failing.

Fixes #58272
Fixes #58660
Fixes #58312
Fixes #59782
Fixes #59784

Closes NEXT-1809

ztanner · 2023-12-20T04:12:09Z

Current dependencies on/for this PR:

canary
- PR fix parallel catch-all route normalization #59791 👈

This stack of pull requests is managed by Graphite.

ijjk · 2023-12-20T04:16:35Z

Tests Passed

ijjk · 2023-12-20T04:18:05Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js 12-19-fix_parallel_catch-all_route_normalization	Change
buildDuration	12.7s	12.7s	N/A
buildDurationCached	7.2s	6.2s	N/A
nodeModulesSize	202 MB	202 MB	⚠️ +6.98 kB
nextStartRea..uration (ms)	422ms	425ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js 12-19-fix_parallel_catch-all_route_normalization	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.2 kB	28.3 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.7 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 12-19-fix_parallel_catch-all_route_normalization	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js 12-19-fix_parallel_catch-all_route_normalization	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 12-19-fix_parallel_catch-all_route_normalization	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 12-19-fix_parallel_catch-all_route_normalization	Change
index.html gzip	529 B	526 B	N/A
link.html gzip	541 B	539 B	N/A
withRouter.html gzip	525 B	522 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js 12-19-fix_parallel_catch-all_route_normalization	Change
edge-ssr.js gzip	93.7 kB	93.7 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 12-19-fix_parallel_catch-all_route_normalization	Change
middleware-b..fest.js gzip	624 B	626 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 12-19-fix_parallel_catch-all_route_normalization	Change
app-page-exp...dev.js gzip	168 kB	168 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.4 kB	89.4 kB	✓
app-page.run...dev.js gzip	138 kB	138 kB	✓
app-page.run..prod.js gzip	88.7 kB	88.7 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.37 kB	9.37 kB	✓
pages-api.ru...dev.js gzip	9.64 kB	9.64 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.5 kB	22.5 kB	✓
pages.runtim..prod.js gzip	21.9 kB	21.9 kB	✓
server.runti..prod.js gzip	49.4 kB	49.4 kB	N/A
Overall change	882 kB	882 kB	✓

Diff details

Diff for server.runtime.prod.js

Diff too large to display

Commit: 44ab019

ztanner · 2023-12-21T05:30:13Z

...es/next/src/server/future/route-matcher-providers/dev/dev-app-page-route-matcher-provider.ts

+    appPaths = Object.fromEntries(
+      Object.entries(appPaths).map(([k, v]) => [k, v.sort()])
+    )


noticed this was in build but not dev, so copied from here

leaning into that abstraction, is there some place where we could have added it only once?

Yeah, need to think about this some more, but it's definitely a bit strange that dev-app-page-route-matcher-provider duplicates this logic but the other matcher providers don't. Seems like this needs to be happening somewhere higher in the stack.

ztanner · 2023-12-21T15:53:23Z

test/e2e/app-dir/parallel-routes-and-interception/parallel-routes-and-interception.test.ts

@@ -187,38 +186,6 @@ createNextDescribe(
        expect(pageText).toContain('parallel/(new)/@baz/nested/page')
      })

-      it('should throw an error when a route groups causes a conflict with a parallel segment', async () => {


this test was reworked and moved here

In it's place I kept the existing implementation but verified that the page slot took precedence over the parallel page.

feedthejim · 2023-12-22T09:45:47Z

packages/next/src/server/load-components.ts

  try {
+    process.env.NEXT_MINIMAL


What is this fixing?

Since I'm no longer throwing a build error if there are two matching children parallel segments, appPaths will technically contain 2 pages and will throw an error when attempting to require it because the FlightManifestPlugin will have only generated a manifest for a single entrypoint. Since the other entry won't be used it seems safe to ignore, and loadClientReferenceManifest could potentially return undefined so I just lifted the try a bit higher.

feedthejim · 2023-12-22T09:46:39Z

...es/next/src/server/future/route-matcher-providers/dev/dev-app-page-route-matcher-provider.ts

+    appPaths = Object.fromEntries(
+      Object.entries(appPaths).map(([k, v]) => [k, v.sort()])
+    )


leaning into that abstraction, is there some place where we could have added it only once?

feedthejim · 2023-12-22T09:47:00Z

packages/next/src/build/normalize-catchall-routes.test.ts

@@ -0,0 +1,99 @@
+import { normalizeCatchAllRoutes } from './normalize-catchall-routes'
+
+describe('normalizeCatchallRoutes', () => {


ijjk added area: tests created-by: Next.js team PRs by the Next.js team type: next labels Dec 20, 2023

ztanner force-pushed the 12-19-fix_parallel_catch-all_route_normalization branch 9 times, most recently from a1f352d to e609c1b Compare December 21, 2023 03:49

ztanner changed the title ~~(wip) fix parallel catch-all route normalization~~ fix parallel catch-all route normalization Dec 21, 2023

ztanner marked this pull request as ready for review December 21, 2023 04:23

ztanner requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, a team and wyattjoh as code owners December 21, 2023 04:23

ztanner commented Dec 21, 2023

View reviewed changes

ztanner force-pushed the 12-19-fix_parallel_catch-all_route_normalization branch from e609c1b to 408e7eb Compare December 21, 2023 18:09

fix parallel catch-all route normalization

44ab019

ztanner force-pushed the 12-19-fix_parallel_catch-all_route_normalization branch from 408e7eb to 44ab019 Compare December 21, 2023 20:02

feedthejim reviewed Dec 22, 2023

View reviewed changes

feedthejim approved these changes Dec 22, 2023

View reviewed changes

ztanner merged commit c4adae8 into canary Dec 22, 2023
72 checks passed

ztanner deleted the 12-19-fix_parallel_catch-all_route_normalization branch December 22, 2023 17:30

github-actions bot added the locked label Jan 6, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix parallel catch-all route normalization #59791

fix parallel catch-all route normalization #59791

ztanner commented Dec 20, 2023 •

edited

ztanner commented Dec 20, 2023

ijjk commented Dec 20, 2023 •

edited

ijjk commented Dec 20, 2023 •

edited

ztanner Dec 21, 2023

feedthejim Dec 22, 2023

ztanner Dec 22, 2023 •

edited

ztanner Dec 21, 2023 •

edited

feedthejim Dec 22, 2023

ztanner Dec 22, 2023

feedthejim Dec 22, 2023

feedthejim Dec 22, 2023

		@@ -0,0 +1,99 @@
		import { normalizeCatchAllRoutes } from './normalize-catchall-routes'

		describe('normalizeCatchallRoutes', () => {

fix parallel catch-all route normalization #59791

fix parallel catch-all route normalization #59791

Conversation

ztanner commented Dec 20, 2023 • edited

What?

Why?

How?

ztanner commented Dec 20, 2023

ijjk commented Dec 20, 2023 • edited

Tests Passed

ijjk commented Dec 20, 2023 • edited

Stats from current PR

ztanner Dec 21, 2023

Choose a reason for hiding this comment

feedthejim Dec 22, 2023

Choose a reason for hiding this comment

ztanner Dec 22, 2023 • edited

Choose a reason for hiding this comment

ztanner Dec 21, 2023 • edited

Choose a reason for hiding this comment

feedthejim Dec 22, 2023

Choose a reason for hiding this comment

ztanner Dec 22, 2023

Choose a reason for hiding this comment

feedthejim Dec 22, 2023

Choose a reason for hiding this comment

feedthejim Dec 22, 2023

Choose a reason for hiding this comment

ztanner commented Dec 20, 2023 •

edited

ijjk commented Dec 20, 2023 •

edited

ijjk commented Dec 20, 2023 •

edited

ztanner Dec 22, 2023 •

edited

ztanner Dec 21, 2023 •

edited