Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch-all and root page throws error of conflicting route if the catch-all is inside routegroup with parallel route. #58312

Closed
1 task done
omerman opened this issue Nov 10, 2023 · 2 comments · Fixed by #59791
Labels
bug Issue was opened via the bug report template. locked

Comments

@omerman
Copy link

omerman commented Nov 10, 2023

Link to the code that reproduces this issue

https://github.com/omerman/catchall-routegroup-parallel

To Reproduce

Start the application, yarn dev, and you'll get an error.

Current vs. Expected behavior

/ route should render Home,
anything else should render the Catcher.

Please help

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: x64
  Version: Darwin Kernel Version 22.6.0: Wed Jul  5 22:21:56 PDT 2023; root:xnu-8796.141.3~6/RELEASE_X86_64
Binaries:
  Node: 18.17.1
  npm: 9.6.7
  Yarn: 1.22.19
  pnpm: 7.29.3
Relevant Packages:
  next: 14.0.3-canary.1
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.1.3
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

App Router

Additional context

No response

@omerman omerman added the bug Issue was opened via the bug report template. label Nov 10, 2023
@omerman
Copy link
Author

omerman commented Nov 11, 2023

might be related to:
#58272

ztanner added a commit that referenced this issue Dec 22, 2023
### 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 #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
Copy link
Contributor

github-actions bot commented Jan 6, 2024

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot added the locked label Jan 6, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2024
agustints pushed a commit to agustints/next.js that referenced this issue Jan 6, 2024
### 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?
vercel#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 vercel#58215 in Turbopack. Once this is done, we should expect the
tests added in this PR to start failing.

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

Closes NEXT-1809
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. locked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant