From 857f4b21c5e864b43fc4eba44a26dc479c04be54 Mon Sep 17 00:00:00 2001 From: agadzik Date: Wed, 1 May 2024 09:35:58 -0400 Subject: [PATCH 1/3] Fixes the issue where the catch-all slug is incorrect --- .../next/src/server/app-render/app-render.tsx | 20 ++++++++++++++----- .../app/@slot/[...catchAll]/page.tsx | 2 +- .../app/foo/[lang]/bar/page.tsx | 7 +++++++ .../parallel-routes-breadcrumbs.test.ts | 15 ++++++++++++++ 4 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 test/e2e/app-dir/parallel-routes-breadcrumbs/app/foo/[lang]/bar/page.tsx diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 8a598b6cc3737..7b87756beeeac 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -207,6 +207,7 @@ export type CreateSegmentPath = (child: FlightSegmentPath) => FlightSegmentPath */ function makeGetDynamicParamFromSegment( params: { [key: string]: any }, + pagePath: string, flightRouterState: FlightRouterState | undefined ): GetDynamicParamFromSegment { return function getDynamicParamFromSegment( @@ -239,11 +240,19 @@ function makeGetDynamicParamFromSegment( segmentParam.type === 'optional-catchall' || segmentParam.type === 'catchall' ) { - // If we weren't able to match the segment to a URL param, and we have a catch-all route, - // provide all of the known params (in array format) to the route - // It should be safe to assume the order of these params is consistent with the order of the segments. - // However, if not, we could re-parse the `pagePath` with `getRouteRegex` and iterate over the positional order. - value = Object.values(params).map((i) => encodeURIComponent(i)) + // derive the value from the page path, replacing any dynamic params with the actual values + value = pagePath + .split('/') + .slice(1) + .map((part) => { + const match = /^\[(.+)\]$/.exec(part) + + if (match) { + return params[match[1]] + } + + return part + }) const hasValues = value.length > 0 const type = dynamicParamTypes[segmentParam.type] return { @@ -795,6 +804,7 @@ async function renderToHTMLOrFlightImpl( const getDynamicParamFromSegment = makeGetDynamicParamFromSegment( params, + pagePath, // `FlightRouterState` is unconditionally provided here because this method uses it // to extract dynamic params as a fallback if they're not present in the path. parsedFlightRouterState diff --git a/test/e2e/app-dir/parallel-routes-breadcrumbs/app/@slot/[...catchAll]/page.tsx b/test/e2e/app-dir/parallel-routes-breadcrumbs/app/@slot/[...catchAll]/page.tsx index f9c359f5c0712..bb230623f6c6a 100644 --- a/test/e2e/app-dir/parallel-routes-breadcrumbs/app/@slot/[...catchAll]/page.tsx +++ b/test/e2e/app-dir/parallel-routes-breadcrumbs/app/@slot/[...catchAll]/page.tsx @@ -1,4 +1,4 @@ -export default function Page({ params: { catchAll } }) { +export default function Page({ params: { catchAll = [] } }) { return (

Parallel Route!

diff --git a/test/e2e/app-dir/parallel-routes-breadcrumbs/app/foo/[lang]/bar/page.tsx b/test/e2e/app-dir/parallel-routes-breadcrumbs/app/foo/[lang]/bar/page.tsx new file mode 100644 index 0000000000000..63c35624b21cd --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-breadcrumbs/app/foo/[lang]/bar/page.tsx @@ -0,0 +1,7 @@ +export default function StaticPage() { + return ( +
+

/foo/[lang]/bar Page!

+
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-breadcrumbs/parallel-routes-breadcrumbs.test.ts b/test/e2e/app-dir/parallel-routes-breadcrumbs/parallel-routes-breadcrumbs.test.ts index 63722ed214f11..e1e09ab7f1825 100644 --- a/test/e2e/app-dir/parallel-routes-breadcrumbs/parallel-routes-breadcrumbs.test.ts +++ b/test/e2e/app-dir/parallel-routes-breadcrumbs/parallel-routes-breadcrumbs.test.ts @@ -44,4 +44,19 @@ describe('parallel-routes-breadcrumbs', () => { expect(await slot.text()).toContain('Album: album2') expect(await slot.text()).toContain('Track: track3') }) + + it('should render the breadcrumbs correctly with the non-dynamic route segments', async () => { + const browser = await next.browser('/foo/en/bar') + const slot = await browser.waitForElementByCss('#slot') + + expect(await browser.elementByCss('h1').text()).toBe('Parallel Route!') + expect(await browser.elementByCss('h2').text()).toBe( + '/foo/[lang]/bar Page!' + ) + + // verify slot is rendering the params + expect(await slot.text()).toContain('Artist: foo') + expect(await slot.text()).toContain('Album: en') + expect(await slot.text()).toContain('Track: bar') + }) }) From 8d751cda95b246a4cac7601f9add2d004aa6680c Mon Sep 17 00:00:00 2001 From: agadzik Date: Wed, 1 May 2024 12:19:21 -0400 Subject: [PATCH 2/3] Fix optional catchall routes --- .../next/src/server/app-render/app-render.tsx | 46 +++++++++++++++---- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 7b87756beeeac..8c2d52a748a30 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -109,6 +109,7 @@ import { } from '../client-component-renderer-logger' import { createServerModuleMap } from './action-utils' import { isNodeNextRequest } from '../base-http/helpers' +import { getRouteRegex } from '../../shared/lib/router/utils/route-regex' export type GetDynamicParamFromSegment = ( // [slug] / [[slug]] / [...slug] @@ -235,15 +236,36 @@ function makeGetDynamicParamFromSegment( } if (!value) { - // Handle case where optional catchall does not have a value, e.g. `/dashboard/[[...slug]]` when requesting `/dashboard` - if ( - segmentParam.type === 'optional-catchall' || - segmentParam.type === 'catchall' - ) { - // derive the value from the page path, replacing any dynamic params with the actual values + const dynamicParamType = dynamicParamTypes[segmentParam.type] + const routeRegex = getRouteRegex(pagePath) + const dynamicParams = routeRegex.groups + const dynamicParamEntries = Object.entries(dynamicParams) + const isCatchall = segmentParam.type === 'catchall' + const isOptionalCatchall = segmentParam.type === 'optional-catchall' + // does `pagePath` contain any `[[...abc]]` segments + const doesPagePathContainOptionalCatchAllParam = dynamicParamEntries.some( + ([, dynamicParam]) => dynamicParam.optional && dynamicParam.repeat + ) + + // handle the case where an optional catchall does not have a value, + // e.g. `/dashboard/[[...slug]]` when requesting `/dashboard` + if (isOptionalCatchall && doesPagePathContainOptionalCatchAllParam) { + return { + param: key, + value: null, + type: dynamicParamType, + treeSegment: [key, '', dynamicParamType], + } + } + + // handle the case where a catchall or optional catchall does not have a value, + // e.g. `/foo/bar/hello` and `@slot/[...catchall]` or `@slot/[[...catchall]]` is matched + if (isCatchall || isOptionalCatchall) { value = pagePath .split('/') + // remove the first empty string .slice(1) + // replace any dynamic params with the actual values .map((part) => { const match = /^\[(.+)\]$/.exec(part) @@ -253,16 +275,22 @@ function makeGetDynamicParamFromSegment( return part }) + const hasValues = value.length > 0 - const type = dynamicParamTypes[segmentParam.type] + return { param: key, value: hasValues ? value : null, - type: type, + type: dynamicParamType, // This value always has to be a string. - treeSegment: [key, hasValues ? value.join('/') : '', type], + treeSegment: [ + key, + hasValues ? value.join('/') : '', + dynamicParamType, + ], } } + return findDynamicParamFromRouterState(flightRouterState, segment) } From 4b738184ed0a756c83f0ff0efd12a5e51b7471e9 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Wed, 1 May 2024 14:59:59 -0700 Subject: [PATCH 3/3] simplify a bit --- .../next/src/server/app-render/app-render.tsx | 57 +++++++------------ .../shared/lib/router/utils/route-regex.ts | 2 +- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 8c2d52a748a30..39a2ae918b9d0 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -109,7 +109,7 @@ import { } from '../client-component-renderer-logger' import { createServerModuleMap } from './action-utils' import { isNodeNextRequest } from '../base-http/helpers' -import { getRouteRegex } from '../../shared/lib/router/utils/route-regex' +import { parseParameter } from '../../shared/lib/router/utils/route-regex' export type GetDynamicParamFromSegment = ( // [slug] / [[slug]] / [...slug] @@ -236,58 +236,43 @@ function makeGetDynamicParamFromSegment( } if (!value) { - const dynamicParamType = dynamicParamTypes[segmentParam.type] - const routeRegex = getRouteRegex(pagePath) - const dynamicParams = routeRegex.groups - const dynamicParamEntries = Object.entries(dynamicParams) const isCatchall = segmentParam.type === 'catchall' const isOptionalCatchall = segmentParam.type === 'optional-catchall' - // does `pagePath` contain any `[[...abc]]` segments - const doesPagePathContainOptionalCatchAllParam = dynamicParamEntries.some( - ([, dynamicParam]) => dynamicParam.optional && dynamicParam.repeat - ) - // handle the case where an optional catchall does not have a value, - // e.g. `/dashboard/[[...slug]]` when requesting `/dashboard` - if (isOptionalCatchall && doesPagePathContainOptionalCatchAllParam) { - return { - param: key, - value: null, - type: dynamicParamType, - treeSegment: [key, '', dynamicParamType], + if (isCatchall || isOptionalCatchall) { + const dynamicParamType = dynamicParamTypes[segmentParam.type] + // handle the case where an optional catchall does not have a value, + // e.g. `/dashboard/[[...slug]]` when requesting `/dashboard` + if (isOptionalCatchall) { + return { + param: key, + value: null, + type: dynamicParamType, + treeSegment: [key, '', dynamicParamType], + } } - } - // handle the case where a catchall or optional catchall does not have a value, - // e.g. `/foo/bar/hello` and `@slot/[...catchall]` or `@slot/[[...catchall]]` is matched - if (isCatchall || isOptionalCatchall) { + // handle the case where a catchall or optional catchall does not have a value, + // e.g. `/foo/bar/hello` and `@slot/[...catchall]` or `@slot/[[...catchall]]` is matched value = pagePath .split('/') // remove the first empty string .slice(1) // replace any dynamic params with the actual values - .map((part) => { - const match = /^\[(.+)\]$/.exec(part) + .map((pathSegment) => { + const param = parseParameter(pathSegment) - if (match) { - return params[match[1]] - } - - return part + // if the segment matches a param, return the param value + // otherwise, it's a static segment, so just return that + return params[param.key] ?? param.key }) - const hasValues = value.length > 0 - return { param: key, - value: hasValues ? value : null, + value, type: dynamicParamType, // This value always has to be a string. - treeSegment: [ - key, - hasValues ? value.join('/') : '', - dynamicParamType, - ], + treeSegment: [key, value.join('/'), dynamicParamType], } } diff --git a/packages/next/src/shared/lib/router/utils/route-regex.ts b/packages/next/src/shared/lib/router/utils/route-regex.ts index f6bff51cbfd03..f46c94e112dd0 100644 --- a/packages/next/src/shared/lib/router/utils/route-regex.ts +++ b/packages/next/src/shared/lib/router/utils/route-regex.ts @@ -25,7 +25,7 @@ export interface RouteRegex { * - `[foo]` -> `{ key: 'foo', repeat: false, optional: true }` * - `bar` -> `{ key: 'bar', repeat: false, optional: false }` */ -function parseParameter(param: string) { +export function parseParameter(param: string) { const optional = param.startsWith('[') && param.endsWith(']') if (optional) { param = param.slice(1, -1)