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

fix parallel catch-all route normalization #59791

Merged
merged 1 commit into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions packages/next/src/build/normalize-catchall-routes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { normalizeCatchAllRoutes } from './normalize-catchall-routes'

describe('normalizeCatchallRoutes', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful

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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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()])
)
Comment on lines +63 to +65
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@ztanner ztanner Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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
Original file line number Diff line number Diff line change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this fixing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

? // @ts-ignore
__non_webpack_require__(manifestPath)
: require(manifestPath)
return (globalThis as any).__RSC_MANIFEST[
entryName
] as ClientReferenceManifest
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import Link from 'next/link'

export default function Home() {
return (
<div>
Home <Link href="/foo">To /foo</Link>
</div>
)
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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>
)
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {}

module.exports = nextConfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page({ params }: { params: { ids: string[] } }) {
return <div>Intercepted Modal Page. Id: {params.ids}</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Default() {
return <div>default @modal</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>Root Catch-All Page</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Default() {
return <div>default root</div>
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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>
)
}
Original file line number Diff line number Diff line change
@@ -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/
)
})
}
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {}

module.exports = nextConfig
Loading