Skip to content

Commit

Permalink
fix parallel catch-all route normalization (vercel#59791)
Browse files Browse the repository at this point in the history
### 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
  • Loading branch information
ztanner authored and agustints committed Jan 6, 2024
1 parent da628f4 commit f25ffde
Show file tree
Hide file tree
Showing 39 changed files with 510 additions and 29 deletions.
99 changes: 99 additions & 0 deletions packages/next/src/build/normalize-catchall-routes.test.ts
@@ -0,0 +1,99 @@
import { normalizeCatchAllRoutes } from './normalize-catchall-routes'

describe('normalizeCatchallRoutes', () => {
it('should not add the catch-all to the interception route', () => {
const appPaths = {
'/': ['/page'],
'/[...slug]': ['/[...slug]/page'],
'/things/[...ids]': ['/things/[...ids]/page'],
'/(.)things/[...ids]': ['/@modal/(.)things/[...ids]/page'],
}

const initialAppPaths = JSON.parse(JSON.stringify(appPaths))

normalizeCatchAllRoutes(appPaths)

expect(appPaths).toMatchObject(initialAppPaths)
})

it('should add the catch-all route to all matched paths when nested', () => {
const appPaths = {
'/parallel-nested-catchall': ['/parallel-nested-catchall/page'],
'/parallel-nested-catchall/[...catchAll]': [
'/parallel-nested-catchall/[...catchAll]/page',
'/parallel-nested-catchall/@slot/[...catchAll]/page',
],
'/parallel-nested-catchall/bar': ['/parallel-nested-catchall/bar/page'],
'/parallel-nested-catchall/foo': [
'/parallel-nested-catchall/foo/page',
'/parallel-nested-catchall/@slot/foo/page',
],
'/parallel-nested-catchall/foo/[id]': [
'/parallel-nested-catchall/foo/[id]/page',
],
'/parallel-nested-catchall/foo/[...catchAll]': [
'/parallel-nested-catchall/@slot/foo/[...catchAll]/page',
],
}

normalizeCatchAllRoutes(appPaths)

expect(appPaths).toMatchObject({
'/parallel-nested-catchall': ['/parallel-nested-catchall/page'],
'/parallel-nested-catchall/[...catchAll]': [
'/parallel-nested-catchall/[...catchAll]/page',
'/parallel-nested-catchall/@slot/[...catchAll]/page',
],
'/parallel-nested-catchall/bar': [
'/parallel-nested-catchall/bar/page',
'/parallel-nested-catchall/@slot/[...catchAll]/page', // inserted
],
'/parallel-nested-catchall/foo': [
'/parallel-nested-catchall/foo/page',
'/parallel-nested-catchall/@slot/foo/page',
],
'/parallel-nested-catchall/foo/[id]': [
'/parallel-nested-catchall/foo/[id]/page',
'/parallel-nested-catchall/@slot/foo/[...catchAll]/page', // inserted
],
'/parallel-nested-catchall/foo/[...catchAll]': [
'/parallel-nested-catchall/@slot/foo/[...catchAll]/page',
'/parallel-nested-catchall/[...catchAll]/page', // inserted
],
})
})

it('should add the catch-all route to all matched paths at the root', () => {
const appPaths = {
'/': ['/page'],
'/[...catchAll]': ['/[...catchAll]/page', '/@slot/[...catchAll]/page'],
'/bar': ['/bar/page'],
'/foo': ['/foo/page', '/@slot/foo/page'],
'/foo/[id]': ['/foo/[id]/page'],
'/foo/[...catchAll]': ['/@slot/foo/[...catchAll]/page'],
}

normalizeCatchAllRoutes(appPaths)

expect(appPaths).toMatchObject({
'/': [
'/page',
'/@slot/[...catchAll]/page', // inserted
],
'/[...catchAll]': ['/[...catchAll]/page', '/@slot/[...catchAll]/page'],
'/bar': [
'/bar/page',
'/@slot/[...catchAll]/page', // inserted
],
'/foo': ['/foo/page', '/@slot/foo/page'],
'/foo/[id]': [
'/foo/[id]/page',
'/@slot/foo/[...catchAll]/page', // inserted
],
'/foo/[...catchAll]': [
'/@slot/foo/[...catchAll]/page',
'/[...catchAll]/page', //inserted
],
})
})
})
14 changes: 11 additions & 3 deletions packages/next/src/build/normalize-catchall-routes.ts
@@ -1,3 +1,4 @@
import { isInterceptionRouteAppPath } from '../server/future/helpers/interception-routes'
import { AppPathnameNormalizer } from '../server/future/normalizers/built/app/app-pathname-normalizer'

/**
Expand All @@ -21,7 +22,14 @@ export function normalizeCatchAllRoutes(
),
]

for (const appPath of Object.keys(appPaths)) {
// interception routes should only be matched by a single entrypoint
// we don't want to push a catch-all route to an interception route
// because it would mean the interception would be handled by the wrong page component
const filteredAppPaths = Object.keys(appPaths).filter(
(route) => !isInterceptionRouteAppPath(route)
)

for (const appPath of filteredAppPaths) {
for (const catchAllRoute of catchAllRoutes) {
const normalizedCatchAllRoute = normalizer.normalize(catchAllRoute)
const normalizedCatchAllRouteBasePath = normalizedCatchAllRoute.slice(
Expand All @@ -30,9 +38,9 @@ export function normalizeCatchAllRoutes(
)

if (
// first check if the appPath could match the catch-all
// check if the appPath could match the catch-all
appPath.startsWith(normalizedCatchAllRouteBasePath) &&
// then check if there's not already a slot value that could match the catch-all
// check if there's not already a slot value that could match the catch-all
!appPaths[appPath].some((path) => hasMatchedSlots(path, catchAllRoute))
) {
appPaths[appPath].push(catchAllRoute)
Expand Down
25 changes: 20 additions & 5 deletions packages/next/src/build/webpack/loaders/next-app-loader.ts
Expand Up @@ -546,12 +546,27 @@ const nextAppLoader: AppLoader = async function nextAppLoader() {
continue
}

// avoid clobbering existing page segments
// if it's a valid parallel segment, the `children` property will be set appropriately
if (existingChildrenPath && matched.children !== rest[0]) {
throw new Error(
`You cannot have two parallel pages that resolve to the same path. Please check ${existingChildrenPath} and ${appPath}. Refer to the route group docs for more information: https://nextjs.org/docs/app/building-your-application/routing/route-groups`
)
// If we get here, it means we already set a `page` segment earlier in the loop,
// meaning we already matched a page to the `children` parallel segment.
const isIncomingParallelPage = appPath.includes('@')
const hasCurrentParallelPage = existingChildrenPath.includes('@')

if (isIncomingParallelPage) {
// The duplicate segment was for a parallel slot. In this case,
// rather than throwing an error, we can ignore it since this can happen for valid reasons.
// For example, when we attempt to normalize catch-all routes, we'll push potential slot matches so
// that they are available in the loader tree when we go to render the page.
// We only need to throw an error if the duplicate segment was for a regular page.
// For example, /app/(groupa)/page & /app/(groupb)/page is an error since it corresponds
// with the same path.
continue
} else if (!hasCurrentParallelPage && !isIncomingParallelPage) {
// Both the current `children` and the incoming `children` are regular pages.
throw new Error(
`You cannot have two parallel pages that resolve to the same path. Please check ${existingChildrenPath} and ${appPath}. Refer to the route group docs for more information: https://nextjs.org/docs/app/building-your-application/routing/route-groups`
)
}
}

existingChildrenPath = appPath
Expand Down
Expand Up @@ -34,7 +34,7 @@ export class DevAppPageRouteMatcherProvider extends FileCacheRouteMatcherProvide
{ page: string; pathname: string; bundlePath: string }
>()
const routeFilenames = new Array<string>()
const appPaths: Record<string, string[]> = {}
let appPaths: Record<string, string[]> = {}
for (const filename of files) {
// If the file isn't a match for this matcher, then skip it.
if (!this.expression.test(filename)) continue
Expand All @@ -59,6 +59,11 @@ export class DevAppPageRouteMatcherProvider extends FileCacheRouteMatcherProvide

normalizeCatchAllRoutes(appPaths)

// Make sure to sort parallel routes to make the result deterministic.
appPaths = Object.fromEntries(
Object.entries(appPaths).map(([k, v]) => [k, v.sort()])
)

const matchers: Array<AppPageRouteMatcher> = []
for (const filename of routeFilenames) {
// Grab the cached values (and the appPaths).
Expand Down
8 changes: 4 additions & 4 deletions packages/next/src/server/load-components.ts
Expand Up @@ -86,11 +86,11 @@ async function loadClientReferenceManifest(
manifestPath: string,
entryName: string
): Promise<ClientReferenceManifest | undefined> {
process.env.NEXT_MINIMAL
? // @ts-ignore
__non_webpack_require__(manifestPath)
: require(manifestPath)
try {
process.env.NEXT_MINIMAL
? // @ts-ignore
__non_webpack_require__(manifestPath)
: require(manifestPath)
return (globalThis as any).__RSC_MANIFEST[
entryName
] as ClientReferenceManifest
Expand Down
@@ -0,0 +1,9 @@
import Link from 'next/link'

export default function Home() {
return (
<div>
Home <Link href="/foo">To /foo</Link>
</div>
)
}
@@ -0,0 +1,9 @@
import Link from 'next/link'

export default function Home() {
return (
<div>
Home <Link href="/foo">To /foo</Link>
</div>
)
}
11 changes: 11 additions & 0 deletions test/e2e/app-dir/conflicting-page-segments/app/layout.tsx
@@ -0,0 +1,11 @@
import React from 'react'

export default function Root({ children }: { children: React.ReactNode }) {
return (
<html>
<body>
<div id="children">{children}</div>
</body>
</html>
)
}
@@ -0,0 +1,31 @@
import { createNextDescribe } from 'e2e-utils'
import { check } from 'next-test-utils'

createNextDescribe(
'conflicting-page-segments',
{
files: __dirname,
// we skip start because the build will fail and we won't be able to catch it
// start is re-triggered but caught in the assertions below
skipStart: true,
},
({ next, isNextDev }) => {
it('should throw an error when a route groups causes a conflict with a parallel segment', async () => {
if (isNextDev) {
await next.start()
const html = await next.render('/')

expect(html).toContain(
'You cannot have two parallel pages that resolve to the same path.'
)
} else {
await expect(next.start()).rejects.toThrow('next build failed')

await check(
() => next.cliOutput,
/You cannot have two parallel pages that resolve to the same path\. Please check \/\(group-a\)\/page and \/\(group-b\)\/page\./i
)
}
})
}
)
6 changes: 6 additions & 0 deletions test/e2e/app-dir/conflicting-page-segments/next.config.js
@@ -0,0 +1,6 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {}

module.exports = nextConfig
@@ -0,0 +1,3 @@
export default function Page({ params }: { params: { ids: string[] } }) {
return <div>Intercepted Modal Page. Id: {params.ids}</div>
}
@@ -0,0 +1,3 @@
export default function Default() {
return <div>default @modal</div>
}
@@ -0,0 +1,3 @@
export default function Page() {
return <div>Root Catch-All Page</div>
}
@@ -0,0 +1,3 @@
export default function Default() {
return <div>default root</div>
}
@@ -0,0 +1,3 @@
export default function Page({ params }: { params: { ids: string[] } }) {
return <div>Regular Item Page. Id: {params.ids}</div>
}
18 changes: 18 additions & 0 deletions test/e2e/app-dir/interception-routes-root-catchall/app/layout.tsx
@@ -0,0 +1,18 @@
import React from 'react'

export default function Root({
children,
modal,
}: {
children: React.ReactNode
modal: React.ReactNode
}) {
return (
<html>
<body>
<div id="children">{children}</div>
<div id="slot">{modal}</div>
</body>
</html>
)
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/interception-routes-root-catchall/app/page.tsx
@@ -0,0 +1,10 @@
import Link from 'next/link'

export default async function Home() {
return (
<div>
<Link href="/items/1">Open Items #1 (Intercepted)</Link>
<Link href="/foobar">Go to Catch-All Page</Link>
</div>
)
}
@@ -0,0 +1,41 @@
import { createNextDescribe } from 'e2e-utils'
import { check } from 'next-test-utils'

createNextDescribe(
'interception-routes-root-catchall',
{
files: __dirname,
},
({ next }) => {
it('should support having a root catch-all and a catch-all in a parallel route group', async () => {
const browser = await next.browser('/')
await browser.elementByCss('[href="/items/1"]').click()

// this triggers the /items route interception handling
await check(
() => browser.elementById('slot').text(),
/Intercepted Modal Page. Id: 1/
)
await browser.refresh()

// no longer intercepted, using the page
await check(() => browser.elementById('slot').text(), /default @modal/)
await check(
() => browser.elementById('children').text(),
/Regular Item Page. Id: 1/
)
})

it('should handle non-intercepted catch-all pages', async () => {
const browser = await next.browser('/')

// there's no explicit page for /foobar. This will trigger the catchall [...slug] page
await browser.elementByCss('[href="/foobar"]').click()
await check(() => browser.elementById('slot').text(), /default @modal/)
await check(
() => browser.elementById('children').text(),
/Root Catch-All Page/
)
})
}
)
@@ -0,0 +1,6 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {}

module.exports = nextConfig

0 comments on commit f25ffde

Please sign in to comment.