Skip to content

Commit

Permalink
ensure more specific default segments match parallel routes before …
Browse files Browse the repository at this point in the history
…greedier catch-all segments
  • Loading branch information
ztanner committed Jan 4, 2024
1 parent 91dba7d commit df109cf
Show file tree
Hide file tree
Showing 17 changed files with 139 additions and 13 deletions.
4 changes: 3 additions & 1 deletion packages/next/src/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,9 @@ export default async function build(
validFileMatcher.isAppRouterPage(absolutePath) ||
// For now we only collect the root /not-found page in the app
// directory as the 404 fallback
validFileMatcher.isRootNotFound(absolutePath),
validFileMatcher.isRootNotFound(absolutePath) ||
// Default slots are also valid pages, and need to be considered during path normalization
validFileMatcher.isDefaultSlot(absolutePath),
ignorePartFilter: (part) => part.startsWith('_'),
})
)
Expand Down
28 changes: 27 additions & 1 deletion packages/next/src/build/normalize-catchall-routes.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { isDefaultRoute } from '../lib/is-default-route'
import { isInterceptionRouteAppPath } from '../server/future/helpers/interception-routes'
import { AppPathnameNormalizer } from '../server/future/normalizers/built/app/app-pathname-normalizer'

Expand Down Expand Up @@ -28,6 +29,7 @@ export function normalizeCatchAllRoutes(
const filteredAppPaths = Object.keys(appPaths).filter(
(route) => !isInterceptionRouteAppPath(route)
)
const defaultPaths = new Set(filteredAppPaths.filter(isDefaultRoute))

for (const appPath of filteredAppPaths) {
for (const catchAllRoute of catchAllRoutes) {
Expand All @@ -41,7 +43,11 @@ export function normalizeCatchAllRoutes(
// 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))
!appPaths[appPath].some((path) =>
hasMatchedSlots(path, catchAllRoute)
) &&
// check if there's not already a default route that would match in place of the catch-all
!hasMatchedDefault(appPath, defaultPaths)
) {
appPaths[appPath].push(catchAllRoute)
}
Expand All @@ -62,6 +68,26 @@ function hasMatchedSlots(path1: string, path2: string): boolean {
return true
}

/**
* Checks if the given path would be matched by a default route.
* This is to avoid adding a catch-all route to a path that should be matched by a default route.
*/
function hasMatchedDefault(
pathToCheck: string,
defaultPaths: Set<string>
): boolean {
const pathSegments = pathToCheck.split('/')

for (let i = 1; i <= pathSegments.length; i++) {
const partialRoute = pathSegments.slice(0, i).join('/')
if (defaultPaths.has(partialRoute + '/default')) {
return true
}
}

return false
}

const catchAllRouteRegex = /\[?\[\.\.\./

function isCatchAllRoute(pathname: string): boolean {
Expand Down
10 changes: 8 additions & 2 deletions packages/next/src/export/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import isError from '../lib/is-error'
import { needsExperimentalReact } from '../lib/needs-experimental-react'
import { formatManifest } from '../build/manifests/formatter/format-manifest'
import { validateRevalidate } from '../server/lib/patch-fetch'
import { isDefaultRoute } from '../lib/is-default-route'

function divideSegments(number: number, segments: number): number[] {
const result = []
Expand Down Expand Up @@ -302,7 +303,7 @@ export async function exportAppImpl(

let hasApiRoutes = false
for (const page of pages) {
// _document and _app are not real pages
// _document, _app, and default are not real pages
// _error is exported as 404.html later on
// API Routes are Node.js functions

Expand All @@ -311,7 +312,12 @@ export async function exportAppImpl(
continue
}

if (page === '/_document' || page === '/_app' || page === '/_error') {
if (
page === '/_document' ||
page === '/_app' ||
page === '/_error' ||
isDefaultRoute(page)
) {
continue
}

Expand Down
3 changes: 3 additions & 0 deletions packages/next/src/lib/is-default-route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function isDefaultRoute(value?: string) {
return value?.endsWith('/default')
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ export class DevAppPageRouteMatcherProvider extends FileCacheRouteMatcherProvide

this.normalizers = new DevAppNormalizers(appDir, extensions)

// Match any page file that ends with `/page.${extension}` under the app
// Match any page file that ends with `/page.${extension}` or `/default.${extension}` under the app
// directory.
this.expression = new RegExp(`[/\\\\]page\\.(?:${extensions.join('|')})$`)
this.expression = new RegExp(
`[/\\\\](page|default)\\.(?:${extensions.join('|')})$`
)
}

protected async transform(
Expand Down
5 changes: 5 additions & 0 deletions packages/next/src/server/lib/find-page-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ export function createValidFileMatcher(
return validExtensionFileRegex.test(filePath) || isMetadataFile(filePath)
}

function isDefaultSlot(filePath: string) {
return filePath.endsWith(`default.${pageExtensions[0]}`)
}

function isRootNotFound(filePath: string) {
if (!appDirPath) {
return false
Expand All @@ -143,5 +147,6 @@ export function createValidFileMatcher(
isAppRouterPage,
isMetadataFile,
isRootNotFound,
isDefaultSlot,
}
}
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]/@slot/[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]/@slot/default.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Foo() {
return <div>/[locale]/nested/[foo]/[bar]/@slot/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]/default.tsx</div>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default function Layout({ children, slot }) {
return (
<>
Children: <div id="nested-children">{children}</div>
Slot: <div id="slot">{slot}</div>
</>
)
}
11 changes: 11 additions & 0 deletions test/e2e/app-dir/parallel-routes-catchall-default/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>
Children: <div id="children">{children}</div>
</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import Link from 'next/link'

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,44 @@
import { createNextDescribe } from 'e2e-utils'

createNextDescribe(
'parallel-routes-catchall-default',
{
files: __dirname,
},
({ next }) => {
it('should match default 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 slot to match since there's a page defined at this segment
expect(await browser.elementById('slot').text()).toBe(
'/[locale]/nested/[foo]/[bar]/@slot/page.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'
)

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

This file was deleted.

0 comments on commit df109cf

Please sign in to comment.