Skip to content

Commit

Permalink
fix parallel route top-level catch-all normalization logic to support…
Browse files Browse the repository at this point in the history
… nested explicit (non-catchall) slot routes (#60776)

Fix NEXT-2165

### What?

Addresses the limitation of #60240, where a dummy `default` file is
required in parallel route child slot to prevent errors in dev server
rendering (`TypeError: Cannot read properties of undefined (reading
'clientModules')`) as well as errors in build and deploy (`Error:
ENOENT: no such file or directory, lstat
‘/vercel/path0/.next/server/app/parallel-route/[section]/@part/[partSlug]/page_client-reference-manifest.js’`)

Without the `default.tsx`, builds and deployments will fail with:

<img width="956" alt="CleanShot 2024-01-18 at 02 12 36@2x"
src="https://github.com/vercel/next.js/assets/179761/80ba61bd-6ec0-4b16-a393-dc9375227e19">

local dev server will also crash with:

<img width="986" alt="CleanShot 2024-01-18 at 02 13 19@2x"
src="https://github.com/vercel/next.js/assets/179761/cc500a32-b2f8-47b4-999e-e57cf5141b2f">

> TypeError: Cannot read properties of undefined (reading
'clientModules')


### Why?

Since `default.tsx` is not a compulsory when you have slot that are
specific and ends with a dynamic route segment, this PR extends support
so that it is possible mixing catch-all routes with specific
non-catchall routes without requiring an additional `default.tsx` .

This PR will allow the following test cases to pass:

```
it('should not add the catch-all route to segments that have a more specific [dynamicRoute]', () => {
    const appPaths = {
      '/': ['/page'],
      '/[[...catchAll]]': ['/[[...catchAll]]/page'],
      '/nested/[foo]/[bar]/default': [
        '/nested/[foo]/[bar]/default',
        '/nested/[foo]/[bar]/@slot0/default',
        '/nested/[foo]/[bar]/@slot2/default',
      ],
      '/nested/[foo]/[bar]': [
        '/nested/[foo]/[bar]/@slot0/page',
        '/nested/[foo]/[bar]/@slot1/page',
      ],
      '/nested/[foo]/[bar]/[baz]': [
        '/nested/[foo]/[bar]/@slot0/[baz]/page',
        '/nested/[foo]/[bar]/@slot1/[baz]/page',
      ],
      '/[locale]/nested/[foo]/[bar]/[baz]/[qux]': [
        '/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page',
      ],
    }

    const initialAppPaths = JSON.parse(JSON.stringify(appPaths))
    normalizeCatchAllRoutes(appPaths)
    expect(appPaths).toMatchObject(initialAppPaths)
  })
...
```

```it('should not add the catch-all route to segments that have a more specific [dynamicRoute]', () => {
    const appPaths = {
      '/': ['/page'],
      '/[[...catchAll]]': ['/[[...catchAll]]/page'],
      '/nested/[foo]/[bar]/default': [
        '/nested/[foo]/[bar]/default',
        '/nested/[foo]/[bar]/@slot0/default',
        '/nested/[foo]/[bar]/@slot2/default',
      ],
      '/nested/[foo]/[bar]': [
        '/nested/[foo]/[bar]/@slot0/page',
        '/nested/[foo]/[bar]/@slot1/page',
      ],
      '/nested/[foo]/[bar]/[baz]': [
        '/nested/[foo]/[bar]/@slot0/[baz]/page',
        '/nested/[foo]/[bar]/@slot1/[baz]/page',
      ],
      '/[locale]/nested/[foo]/[bar]/[baz]/[qux]': [
        '/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page',
      ],
    }
...
```

and allow parallel routes defined in this [code
repro](https://github.com/williamli/nextjs-NEXT-2165) to build.


![image](https://github.com/vercel/next.js/assets/179761/030f4fe1-3a27-41e5-bbd9-bc511f95e5d7)


### How?

`packages/next/src/build/normalize-catchall-routes.ts` is extended to
check `appPath` to see if it is:
1. the route is not a catchall
2. `isMoreSpecific` than the closest `catchAllRoute`.


where `isMoreSpecific` is defined as:

```

function isMoreSpecific(pathname: string, catchAllRoute: string): boolean {
  const pathnameDepth = pathname.split('/').length
  const catchAllRouteDepth = catchAllRoute.split('/').length - 1
  return pathnameDepth > catchAllRouteDepth
}

```

---------

Co-authored-by: Zack Tanner <zacktanner@gmail.com>
  • Loading branch information
williamli and ztanner committed Jan 23, 2024
1 parent 78adcd4 commit 71335a9
Show file tree
Hide file tree
Showing 30 changed files with 358 additions and 3 deletions.
55 changes: 55 additions & 0 deletions packages/next/src/build/normalize-catchall-routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,61 @@ describe('normalizeCatchallRoutes', () => {
expect(appPaths).toMatchObject(initialAppPaths)
})

it('should not add the catch-all route to segments that have a more specific [dynamicRoute]', () => {
const appPaths = {
'/': ['/page'],
'/[[...catchAll]]': ['/[[...catchAll]]/page'],
'/nested/[foo]/[bar]/default': [
'/nested/[foo]/[bar]/default',
'/nested/[foo]/[bar]/@slot0/default',
'/nested/[foo]/[bar]/@slot2/default',
],
'/nested/[foo]/[bar]': [
'/nested/[foo]/[bar]/@slot0/page',
'/nested/[foo]/[bar]/@slot1/page',
],
'/nested/[foo]/[bar]/[baz]': [
'/nested/[foo]/[bar]/@slot0/[baz]/page',
'/nested/[foo]/[bar]/@slot1/[baz]/page',
],
'/[locale]/nested/[foo]/[bar]/[baz]/[qux]': [
'/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page',
],
}

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

// normalize appPaths against catchAlls
normalizeCatchAllRoutes(appPaths)

// ensure values are correct after normalizing
expect(appPaths).toMatchObject(initialAppPaths)
})

it('should not add the catch-all route to non-catchall segments that are more specific', () => {
const appPaths = {
'/': ['/page'],
'/[locale]/[[...catchAll]]': ['/[locale]/[[...catchAll]]/page'],
'/[locale]/nested/default': [
'/[locale]/nested/default',
'/[locale]/nested/@slot0/default',
'/[locale]/nested/@slot1/default',
],
'/[locale]/nested': ['/[locale]/nested/page'],
'/[locale]/nested/bar': ['/[locale]/nested/@slot0/bar/page'],
'/[locale]/nested/foo': ['/[locale]/nested/@slot0/foo/page'],
'/[locale]/nested/baz': ['/[locale]/nested/@slot1/baz/page'],
}

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

// normalize appPaths against catchAlls
normalizeCatchAllRoutes(appPaths)

// ensure values are correct after normalizing
expect(appPaths).toMatchObject(initialAppPaths)
})

it('should not add the catch-all route to a path that has a @children slot', async () => {
const appPaths = {
'/': ['/@children/page', '/@slot/page'],
Expand Down
14 changes: 12 additions & 2 deletions packages/next/src/build/normalize-catchall-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,18 @@ export function normalizeCatchAllRoutes(
0,
normalizedCatchAllRoute.search(catchAllRouteRegex)
)

if (
// check if the appPath could match the catch-all
appPath.startsWith(normalizedCatchAllRouteBasePath) &&
// check if there's not already a slot value that could match the catch-all
!appPaths[appPath].some((path) =>
hasMatchedSlots(path, catchAllRoute)
) &&
// check if the catch-all is not already matched by a default route
!appPaths[`${appPath}/default`]
// check if the catch-all is not already matched by a default route or page route
!appPaths[`${appPath}/default`] &&
// check if appPath is a catch-all OR is not more specific than the catch-all
(isCatchAllRoute(appPath) || !isMoreSpecific(appPath, catchAllRoute))
) {
appPaths[appPath].push(catchAllRoute)
}
Expand Down Expand Up @@ -81,3 +84,10 @@ const catchAllRouteRegex = /\[?\[\.\.\./
function isCatchAllRoute(pathname: string): boolean {
return pathname.includes('[...') || pathname.includes('[[...')
}

// test to see if a path is more specific than a catch-all route
function isMoreSpecific(pathname: string, catchAllRoute: string): boolean {
const pathnameDepth = pathname.split('/').length
const catchAllRouteDepth = catchAllRoute.split('/').length - 1
return pathnameDepth > catchAllRouteDepth
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ createNextDescribe(
'/[locale]/nested/[foo]/[bar]/default.tsx'
)

// however we do have a slot for the `[baz]` segment and so we expect that to no match
// however we do have a slot for the `[baz]` segment and so we expect that to match
expect(await browser.elementById('slot').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot/[baz]/page.tsx'
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/[[...catchAll]]/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page({ params }) {
return <div>/[locale]/nested/[foo]/[bar]/@slot0/[baz]/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page({ params }) {
return <div>/[locale]/nested/[foo]/[bar]/@slot0/default.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/nested/[foo]/[bar]/@slot0/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page({ params }) {
return <div>/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page({ params }) {
return <div>/[locale]/nested/[foo]/[bar]/@slot1/[baz]/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/nested/[foo]/[bar]/@slot1/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/nested/[foo]/[bar]/@slot2/default.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/nested/[foo]/[bar]/default.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export default function Layout({ children, slot0, slot1, slot2 }) {
return (
<>
Children: <div id="nested-children">{children}</div>
Slot0: <div id="slot0">{slot0}</div>
Slot1: <div id="slot1">{slot1}</div>
Slot2: <div id="slot2">{slot2}</div>
</>
)
}
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>
Children: <div id="children">{children}</div>
</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default async function Home() {
return <div>Root Page</div>
}
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,87 @@
import { createNextDescribe } from 'e2e-utils'

createNextDescribe(
'parallel-routes-catchall-dynamic-segment',
{
files: __dirname,
},
({ next }) => {
it('should match default and dynamic segment paths before catch-all', async () => {
let browser = await next.browser('/en/nested')

// we have a top-level catch-all but the /nested dir doesn't have a default/page until the /[foo]/[bar] segment
// so we expect the top-level catch-all to render
expect(await browser.elementById('children').text()).toBe(
'/[locale]/[[...catchAll]]/page.tsx'
)

browser = await next.browser('/en/nested/foo/bar')

// we're now at the /[foo]/[bar] segment, so we expect the matched page to be the default (since there's no page defined)
expect(await browser.elementById('nested-children').text()).toBe(
'/[locale]/nested/[foo]/[bar]/default.tsx'
)

// we expect the slot0 to match since there's a page defined at this segment
expect(await browser.elementById('slot0').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot0/page.tsx'
)

// we expect the slot1 to match since there's a page defined at this segment
expect(await browser.elementById('slot1').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot1/page.tsx'
)

// we expect the slot2 to match since there's a default page defined at this segment
expect(await browser.elementById('slot2').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot2/default.tsx'
)

browser = await next.browser('/en/nested/foo/bar/baz')

// the page slot should still be the one matched at the /[foo]/[bar] segment because it's the default and we
// didn't define a page at the /[foo]/[bar]/[baz] segment
expect(await browser.elementById('nested-children').text()).toBe(
'/[locale]/nested/[foo]/[bar]/default.tsx'
)

// we do have a slot for the `[baz]` dynamic segment in slot0 and so we expect that to match
expect(await browser.elementById('slot0').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot0/[baz]/page.tsx'
)

// we do have a slot for the `[baz]` dynamic segment in slot1 and so we expect that to match
expect(await browser.elementById('slot1').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot1/[baz]/page.tsx'
)

// we do not have a slot for the `[baz]` dynamic segment in slot2 and so the default page is matched
expect(await browser.elementById('slot2').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot2/default.tsx'
)

browser = await next.browser('/en/nested/foo/bar/baz/qux')

// the page slot should still be the one matched at the /[foo]/[bar] segment because it's the default and we
// didn't define a page at the /[foo]/[bar]/[baz]/[qux] segment
expect(await browser.elementById('nested-children').text()).toBe(
'/[locale]/nested/[foo]/[bar]/default.tsx'
)

// we do not have a slot for the `[baz]/[qux]` dynamic segment in slot0 and so we expect the default page at `@slot0/` to be returned
expect(await browser.elementById('slot0').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot0/default.tsx'
)

// we do have a slot for the `[baz]/[qux]` dynamic segment in slot1 and so we expect that to no match
expect(await browser.elementById('slot1').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page.tsx'
)

// we do not have a slot for the `[baz]/[qux]` dynamic segment in slot2 and so we expect the default page at `@slot2/` to be returned
expect(await browser.elementById('slot2').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot2/default.tsx'
)
})
}
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/[[...catchAll]]/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page({ params }) {
return <div>/[locale]/nested/@slot0/bar/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/nested/@slot0/default.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page({ params }) {
return <div>/[locale]/nested/@slot0/foo/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page({ params }) {
return <div>/[locale]/nested/@slot1/baz/page.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/nested/@slot1/default.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/nested/default.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default function Layout({ children, slot0, slot1 }) {
return (
<>
Children: <div id="nested-children">{children}</div>
Slot0: <div id="slot0">{slot0}</div>
Slot1: <div id="slot1">{slot1}</div>
</>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>/[locale]/nested/page.tsx</div>
}
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>
Children: <div id="children">{children}</div>
</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default async function Home() {
return <div>Root Page</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {}

module.exports = nextConfig
Loading

2 comments on commit 71335a9

@TommySorensen
Copy link

@TommySorensen TommySorensen commented on 71335a9 Feb 22, 2024

Choose a reason for hiding this comment

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

@williamli @ztanner Sorry to put a comment here, but this was only place i could find a proper place to mention that this still fails in latest canary 14.1.1-canary.69 since the issue is already closed.

image

If i visit the page under /app/[locale]/(checkout)/checkout/@delivery/test then it throws the error.
But if i move the test folder to /app/(checkout)/checkout/@delivery/test (without locale) then it works just fine. I even tried removing my middleware.ts files to make sure nothing 3.party was interferring.

Folder structure is quite similar to what you have in your test.

image

Unfortunately i have unsuccessfully replicated this in a new repo.

@TommySorensen
Copy link

Choose a reason for hiding this comment

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

Even adding a default.tsx to the slug folder makes the whole app go into "Internal Server Error" mode and throwing the error "Error: Catch-all must be the last part of the URL." in server console.
image

Please sign in to comment.