parallel routes: fix catch-all slots being treated as optional catch-all (#61174)

### What
Catch-all parallel slots were being incorrectly matched to the root of their segment. For example, `@foo/[...catchAll]/page` as a parallel route on `/page.tsx` should not match on `/`, but it should match on `/foo`, `/bar`, ...etc

### Why
The catch-all route normalization logic doesn't treat optional catch-all routes differently from catch-all routes. The assumption was if any catch-all route was found, that it should match the path that shared its prefix.

### How
This updates the normalization logic to handle optional-catchall as it was in the original implementation. For regular catch-all, we ensure that the catch-all base path (for `/[...slug]` that'd be `/`) isn't identical to the path we'd match it to.

Fixes #60613
Closes NEXT-2243
This commit is contained in:
Zack Tanner 2024-01-30 15:28:47 -08:00 committed by GitHub
parent 7c4d724e57
commit 31a0edbd1b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 52 additions and 18 deletions

View file

@ -82,10 +82,7 @@ describe('normalizeCatchallRoutes', () => {
// ensure values are correct after normalizing
expect(appPaths).toMatchObject({
'/': [
'/page',
'/@slot/[...catchAll]/page', // inserted
],
'/': ['/page'],
'/[...catchAll]': ['/[...catchAll]/page', '/@slot/[...catchAll]/page'],
'/bar': [
'/bar/page',
@ -103,6 +100,29 @@ describe('normalizeCatchallRoutes', () => {
})
})
it('should only match optional catch-all paths to the "index" of a segment', () => {
const appPaths = {
'/': ['/page'],
'/[[...catchAll]]': ['/@slot/[[...catchAll]]/page'],
'/foo': ['/foo/page'],
'/foo/[[...catchAll]]': ['/foo/@slot/[[...catchAll]]/page'],
}
// normalize appPaths against catchAlls
normalizeCatchAllRoutes(appPaths)
// ensure values are correct after normalizing
expect(appPaths).toMatchObject({
'/': [
'/page',
'/@slot/[[...catchAll]]/page', // inserted
],
'/[[...catchAll]]': ['/@slot/[[...catchAll]]/page'],
'/foo': ['/foo/page', '/@slot/[[...catchAll]]/page'],
'/foo/[[...catchAll]]': ['/foo/@slot/[[...catchAll]]/page'],
})
})
it('should not add the catch-all route to segments that have a more specific default', () => {
const appPaths = {
'/': ['/page'],

View file

@ -47,7 +47,17 @@ export function normalizeCatchAllRoutes(
// 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)
if (isOptionalCatchAll(catchAllRoute)) {
// optional catch-all routes should match both the root segment and any segment after it
// for example, `/[[...slug]]` should match `/` and `/foo` and `/foo/bar`
appPaths[appPath].push(catchAllRoute)
} else if (isCatchAll(catchAllRoute)) {
// regular catch-all (single bracket) should only match segments after it
// for example, `/[...slug]` should match `/foo` and `/foo/bar` but not `/`
if (normalizedCatchAllRouteBasePath !== appPath) {
appPaths[appPath].push(catchAllRoute)
}
}
}
}
}
@ -80,7 +90,15 @@ function isMatchableSlot(segment: string): boolean {
const catchAllRouteRegex = /\[?\[\.\.\./
function isCatchAllRoute(pathname: string): boolean {
return pathname.includes('[...') || pathname.includes('[[...')
return isOptionalCatchAll(pathname) || isCatchAll(pathname)
}
function isOptionalCatchAll(pathname: string): boolean {
return pathname.includes('[[...')
}
function isCatchAll(pathname: string): boolean {
return pathname.includes('[...')
}
// test to see if a path is more specific than a catch-all route

View file

@ -0,0 +1,3 @@
export default function Page() {
return '@slot default'
}

View file

@ -9,7 +9,7 @@ createNextDescribe(
({ next }) => {
it('should match correctly when defining an explicit page & slot', async () => {
const browser = await next.browser('/')
await check(() => browser.elementById('slot').text(), /slot catchall/)
await check(() => browser.elementById('slot').text(), /@slot default/)
await browser.elementByCss('[href="/foo"]').click()
@ -21,7 +21,7 @@ createNextDescribe(
it('should match correctly when defining an explicit page but no slot', async () => {
const browser = await next.browser('/')
await check(() => browser.elementById('slot').text(), /slot catchall/)
await check(() => browser.elementById('slot').text(), /@slot default/)
await browser.elementByCss('[href="/bar"]').click()
@ -37,11 +37,7 @@ createNextDescribe(
it('should match correctly when defining an explicit slot but no page', async () => {
const browser = await next.browser('/')
await check(() => browser.elementById('slot').text(), /slot catchall/)
await check(
() => browser.elementById('slot').text(),
/catchall slot client component/
)
await check(() => browser.elementById('slot').text(), /@slot default/)
await browser.elementByCss('[href="/baz"]').click()
@ -53,11 +49,8 @@ createNextDescribe(
it('should match both the catch-all page & slot', async () => {
const browser = await next.browser('/')
await check(() => browser.elementById('slot').text(), /slot catchall/)
await check(
() => browser.elementById('slot').text(),
/catchall slot client component/
)
await check(() => browser.elementById('slot').text(), /@slot default/)
await browser.elementByCss('[href="/quux"]').click()
// quux doesn't have a page or slot defined. It should use the catch-all for both