Fix: expected "catch all routes" are not matched in “parallel routes" #58368

nonoakij · 2023-11-13T08:30:49Z

This PR fixes a bug where the expected catch-all route would not match if there were multiple "catch-all routes" under the "parallel route”.

The page on the non-parallel routes path matches the catch all routes. that exist on the more detailed path.
However, under parallel routes, it matches the most parent page of all the pages with catch all routes.

For example, if there are files like below:

app/@sidebar/[...catchall]/page.tsx
app/@sidebar/dashboard/[...catchall]/page.tsx

When accessing /foo, it should match app/@sidebar/[...catchall]/page.tsx, and this is working correctly.
However, when accessing /dashboard/foo, it should match app/@sidebar/dashboard/[...catchall]/page.tsx, but app/@sidebar/[...catchall]/page.tsx is being matched instead.

Repository to reproduce

https://github.com/nonoakij/fix-parallel-routes-with-catch-all

Related PR

#58215

ijjk · 2023-11-13T09:01:00Z

Allow CI Workflow Run

approve CI run for commit: c42063b

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

feedthejim

great stuff :) added a few comments

feedthejim · 2023-11-13T08:59:48Z

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

@@ -48,6 +48,8 @@ function hasMatchedSlots(path1: string, path2: string): boolean {
  return true
 }

+const catchAllRouteRegex = /\[\.\.\./;


This should also fix the issue where /foo/[slug]/[...something] would cause errors. However, this regex should also probably support the optional catch all syntax [[.... Mind modifying that?

#58215 (comment) referring to this bug report

@feedthejim I have fixed it.

d24237a

wanna add a test for it?

@feedthejim
I have added that test.

However, when I tried it in my local repository, an error occurred.

This fix may not be enough to fix the optional catchall bug.

> next dev -p 9070 ▲ Next.js 14.0.3-canary.5 - Local: http://localhost:9070 Error: You cannot define a route with the same specificity as a optional catch-all route ("/baz" and "/baz[[...catchAll]]"). at UrlNode._smoosh (/PATH/TO/fix-parallel-routes-with-catch-all/node_modules/next/dist/shared/lib/router/utils/sorted-routes.js:42:23) at /PATH/TO/fix-parallel-routes-with-catch-all/node_modules/next/dist/shared/lib/router/utils/sorted-routes.js:32:68 at Array.map (<anonymous>) at UrlNode._smoosh (/PATH/TO/fix-parallel-routes-with-catch-all/node_modules/next/dist/shared/lib/router/utils/sorted-routes.js:32:38) at UrlNode.smoosh (/PATH/TO/fix-parallel-routes-with-catch-all/node_modules/next/dist/shared/lib/router/utils/sorted-routes.js:16:21) at getSortedRoutes (/PATH/TO/fix-parallel-routes-with-catch-all/node_modules/next/dist/shared/lib/router/utils/sorted-routes.js:167:17) at Watchpack.<anonymous> (/PATH/TO/fix-parallel-routes-with-catch-all/node_modules/next/dist/server/lib/router-utils/setup-dev-bundler.js:1542:65)

$ tree -I 'node_modules' . ├── README.md ├── app │ ├── @sidebar │ │ ├── [...catchAll] │ │ │ └── page.tsx │ │ ├── baz │ │ │ └── [[...catchAll]] │ │ │ └── page.tsx │ │ ├── dashboard │ │ │ └── [...catchAll] │ │ │ └── page.tsx │ │ └── defalt.tsx │ ├── baz │ │ ├── [id] │ │ │ └── page.tsx │ │ └── page.tsx │ ├── dashboard │ │ └── foo │ │ └── page.tsx │ ├── foo │ │ └── page.tsx │ ├── layout.tsx │ └── page.tsx ├── next-env.d.ts ├── next.config.js ├── package-lock.json ├── package.json ├── public │ ├── next.svg │ └── vercel.svg └── tsconfig.json

Ah! Feel free to remove the test then, or keep digging! I'll pick it up later anyway

I did a little digging and the problem seems to be complex.
I'll remove the test case for now, but I'll revisit and investigate further.
If a fix is possible, I'll create a new PR.

I have removed the optional parallel routes test.
7e5b6c2

feedthejim · 2023-11-13T09:00:27Z

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

@@ -12,15 +12,15 @@ export function normalizeCatchAllRoutes(
  normalizer = new AppPathnameNormalizer()
 ) {
  const catchAllRoutes = [
-    ...new Set(Object.values(appPaths).flat().filter(isCatchAllRoute)),
+    ...new Set(Object.values(appPaths).flat().filter(isCatchAllRoute).sort((a,b)=> b.split("/").length - a.split("/").length)),


mind adding a comment here that sorting is important because we want to match the most specific path?

@feedthejim I have added a comment.
4922a63

Still seing 404 issue mentioned in #58215

When using generateStaticParams

ijjk · 2023-11-13T09:06:44Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
buildDuration	10.9s	10.7s	N/A
buildDurationCached	6s	6s	N/A
nodeModulesSize	199 MB	199 MB	⚠️ +1.47 kB
nextStartRea..uration (ms)	427ms	421ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
199-HASH.js gzip	29.2 kB	29.2 kB	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	N/A
494.HASH.js gzip	180 B	181 B	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	242 B	239 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	✓
Overall change	46.9 kB	46.9 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
_app-HASH.js gzip	194 B	195 B	N/A
_error-HASH.js gzip	182 B	181 B	N/A
amp-HASH.js gzip	504 B	506 B	N/A
css-HASH.js gzip	322 B	323 B	N/A
dynamic-HASH.js gzip	2.5 kB	2.5 kB	✓
edge-ssr-HASH.js gzip	253 B	255 B	N/A
head-HASH.js gzip	348 B	347 B	N/A
hooks-HASH.js gzip	369 B	368 B	N/A
image-HASH.js gzip	4.3 kB	4.3 kB	N/A
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.65 kB	2.65 kB	N/A
routerDirect..HASH.js gzip	311 B	311 B	✓
script-HASH.js gzip	384 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.17 kB	3.17 kB	✓

Client Build Manifests

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
_buildManifest.js gzip	486 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
index.html gzip	528 B	526 B	N/A
link.html gzip	540 B	541 B	N/A
withRouter.html gzip	524 B	521 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
edge-ssr.js gzip	92.5 kB	92.5 kB	N/A
page.js gzip	145 kB	145 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
middleware-b..fest.js gzip	624 B	627 B	N/A
middleware-r..fest.js gzip	150 B	151 B	N/A
middleware.js gzip	24.8 kB	24.8 kB	✓
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	26.7 kB	26.7 kB	✓

Next Runtimes

	vercel/next.js canary	nonoakij/next.js fix-parallel-routes-with-catch-all	Change
app-page-exp...dev.js gzip	167 kB	167 kB	✓
app-page-exp..prod.js gzip	93.2 kB	93.2 kB	✓
app-page-tur..prod.js gzip	93.9 kB	93.9 kB	✓
app-page-tur..prod.js gzip	88.5 kB	88.5 kB	✓
app-page.run...dev.js gzip	137 kB	137 kB	✓
app-page.run..prod.js gzip	87.9 kB	87.9 kB	✓
app-route-ex...dev.js gzip	23.8 kB	23.8 kB	✓
app-route-ex..prod.js gzip	16.4 kB	16.4 kB	✓
app-route-tu..prod.js gzip	16.4 kB	16.4 kB	✓
app-route-tu..prod.js gzip	16 kB	16 kB	✓
app-route.ru...dev.js gzip	23.2 kB	23.2 kB	✓
app-route.ru..prod.js gzip	16 kB	16 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.8 kB	21.8 kB	✓
pages.runtim...dev.js gzip	22.5 kB	22.5 kB	✓
pages.runtim..prod.js gzip	21.8 kB	21.8 kB	✓
server.runti..prod.js gzip	48.8 kB	48.8 kB	✓
Overall change	922 kB	922 kB	✓

Commit: f435ea9

ijjk · 2023-11-13T09:07:38Z

Failing test suites

Commit: f435ea9

pnpm test-dev test/e2e/app-dir/app-compilation/index.test.ts

app dir > HMR > should not cause error when removing loading.js

Expand output

● app dir › HMR › should not cause error when removing loading.js

TIMED OUT: hello from new page

Server Error

undefined

  626 |
  627 |   if (hardError) {
> 628 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
      |           ^
  629 |   }
  630 |   return false
  631 | }

  at check (lib/next-test-utils.ts:628:11)
  at Object.<anonymous> (e2e/app-dir/app-compilation/index.test.ts:44:11)

Read more about building and testing Next.js in contributing.md.

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

parallel-routes-and-interception > parallel routes > Should match the catch-all routes of the more specific path, If there is more than one catch-all route

Expand output

● parallel-routes-and-interception › parallel routes › Should match the catch-all routes of the more specific path, If there is more than one catch-all route

TIMED OUT: slot catchall

foo slot

undefined

  626 |
  627 |   if (hardError) {
> 628 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
      |           ^
  629 |   }
  630 |   return false
  631 | }

  at check (lib/next-test-utils.ts:628:11)
  at Object.<anonymous> (e2e/app-dir/parallel-routes-and-interception/parallel-routes-and-interception.test.ts:358:9)

Read more about building and testing Next.js in contributing.md.

feedthejim · 2023-11-13T17:01:34Z

gonna re-run the tests that failed and merge if they look ok. Nice woork

TommySorensen · 2023-11-17T11:00:49Z

I think this fix does not cover all cases. Please see this example where i run v. 14.0.3.
This worked fine in v.14.0.1, but not anymore.

feedthejim · 2023-11-17T11:19:49Z

@TommySorensen can you provide a minimal reproduction?

dbk91 · 2023-11-20T14:23:29Z

@feedthejim I was able to reproduce the same issue in this repository.

It seems if the dev server is freshly started and the dashboard page is opened using hard navigation, it'll render as expected. Otherwise this bug will display if you attempt to navigate to either page subsequently.

I hope this helps.

augustl · 2023-11-27T09:51:48Z

I've got a reproducible case in #58914

nonoakij added 3 commits November 13, 2023 15:52

fix(next): prefer more deeply nested cath all paths

a211fcd

fix(next): basepath may incorporate a dynamic route unexpectedly

f0c396c

test(test): add parallel routes nested routes test

c42063b

nonoakij requested review from timneutkens, ijjk, shuding, huozhi, ztanner, feedthejim, a team and wyattjoh as code owners November 13, 2023 08:30

ijjk added area: tests type: next labels Nov 13, 2023

feedthejim requested changes Nov 13, 2023

View reviewed changes

Merge branch 'canary' into fix-parallel-routes-with-catch-all

845acfa

feedthejim added the CI approved Approve running CI for fork label Nov 13, 2023

nonoakij and others added 6 commits November 13, 2023 18:13

fix(next): consider optional catch all

d24237a

feat(next): add comments

4922a63

test(test): add optional routes test

14ac39d

test(test): remove optional catch all test

7e5b6c2

Merge branch 'canary' into fix-parallel-routes-with-catch-all

ca11c1f

Merge branch 'canary' into fix-parallel-routes-with-catch-all

f435ea9

nonoakij requested a review from feedthejim November 13, 2023 16:59

feedthejim approved these changes Nov 13, 2023

View reviewed changes

feedthejim merged commit 4fe968b into vercel:canary Nov 13, 2023
56 of 58 checks passed

wlechowicz mentioned this pull request Nov 13, 2023

parallel routes: catch all nested under dynamic segment matches to the root #58272

Closed

1 task

nonoakij deleted the fix-parallel-routes-with-catch-all branch November 14, 2023 00:46

itsjavi mentioned this pull request Nov 25, 2023

Multiple levels of catch-all routes with intercepted + parallel routes loads the wrong route on intercept, but the correct route on initial page load #58660

Closed

1 task

github-actions bot added the locked label Dec 11, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: expected "catch all routes" are not matched in “parallel routes" #58368

Fix: expected "catch all routes" are not matched in “parallel routes" #58368

nonoakij commented Nov 13, 2023 •

edited

Loading

ijjk commented Nov 13, 2023 •

edited by feedthejim

Loading

feedthejim left a comment

feedthejim Nov 13, 2023

feedthejim Nov 13, 2023

nonoakij Nov 13, 2023

feedthejim Nov 13, 2023

nonoakij Nov 13, 2023 •

edited

Loading

feedthejim Nov 13, 2023

nonoakij Nov 13, 2023

nonoakij Nov 13, 2023

feedthejim Nov 13, 2023

nonoakij Nov 13, 2023

RootedXpert Nov 14, 2023

ijjk commented Nov 13, 2023 •

edited

Loading

ijjk commented Nov 13, 2023 •

edited

Loading

feedthejim commented Nov 13, 2023

TommySorensen commented Nov 17, 2023

feedthejim commented Nov 17, 2023

dbk91 commented Nov 20, 2023

augustl commented Nov 27, 2023

Fix: expected "catch all routes" are not matched in “parallel routes" #58368

Fix: expected "catch all routes" are not matched in “parallel routes" #58368

Conversation

nonoakij commented Nov 13, 2023 • edited Loading

Repository to reproduce

Related PR

ijjk commented Nov 13, 2023 • edited by feedthejim Loading

feedthejim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonoakij Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijjk commented Nov 13, 2023 • edited Loading

Stats from current PR

ijjk commented Nov 13, 2023 • edited Loading

Failing test suites

feedthejim commented Nov 13, 2023

TommySorensen commented Nov 17, 2023

feedthejim commented Nov 17, 2023

dbk91 commented Nov 20, 2023

augustl commented Nov 27, 2023

nonoakij commented Nov 13, 2023 •

edited

Loading

ijjk commented Nov 13, 2023 •

edited by feedthejim

Loading

nonoakij Nov 13, 2023 •

edited

Loading

ijjk commented Nov 13, 2023 •

edited

Loading

ijjk commented Nov 13, 2023 •

edited

Loading