From 89ad6121657ac3fa59e012474816552045d7efcf Mon Sep 17 00:00:00 2001 From: Andrew Gadzik Date: Thu, 2 May 2024 14:31:39 -0400 Subject: [PATCH 1/2] Fix an issue parsing catchall params (#65277) This should fix the following scenarios, - Given a page defined like `app/foo/[...bar]/page.tsx` - Given a page defined like `app/bar/[[...foo]]/page.tsx` - Given a parallel route defined like `app/@slot/[...catchall]/page.tsx` If you navigate to `/foo/bar` the `params` prop in the parallel route would be ```js params: { catchall: [ 'foo', [ 'bar' ] ] } ``` And if you navigate to `/bar/foo` the `params` prop in the parallel route would be ```js params: { catchall: [ 'bar', '[ ...foo ]' ] } ``` With the fix in place, the `params` prop in the parallel route will be, ```js params: { catchall: [ 'foo', 'bar', ] } ``` And ```js params: { catchall: [ 'bar', 'foo', ] } ``` Respectively --- .../next/src/server/app-render/app-render.tsx | 3 +- .../lib/router/utils/route-regex.test.ts | 38 ++++++++++++++ .../shared/lib/router/utils/route-regex.ts | 52 ++++++++++++++++--- .../app/buzz/[[...fizz]]/page.tsx | 7 +++ .../app/fizz/[...buzz]/page.tsx | 7 +++ .../app/foo/[lang]/bar/page.tsx | 2 +- .../parallel-routes-breadcrumbs.test.ts | 30 +++++++++++ 7 files changed, 130 insertions(+), 9 deletions(-) create mode 100644 test/e2e/app-dir/parallel-routes-breadcrumbs/app/buzz/[[...fizz]]/page.tsx create mode 100644 test/e2e/app-dir/parallel-routes-breadcrumbs/app/fizz/[...buzz]/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 39a2ae918b9d0..3a207806e4bea 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -259,9 +259,8 @@ function makeGetDynamicParamFromSegment( // remove the first empty string .slice(1) // replace any dynamic params with the actual values - .map((pathSegment) => { + .flatMap((pathSegment) => { const param = parseParameter(pathSegment) - // 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 diff --git a/packages/next/src/shared/lib/router/utils/route-regex.test.ts b/packages/next/src/shared/lib/router/utils/route-regex.test.ts index 1b7eb4ae8e8c5..cd50e768b192d 100644 --- a/packages/next/src/shared/lib/router/utils/route-regex.test.ts +++ b/packages/next/src/shared/lib/router/utils/route-regex.test.ts @@ -1,4 +1,5 @@ import { getNamedRouteRegex } from './route-regex' +import { parseParameter } from './route-regex' describe('getNamedRouteRegex', () => { it('should handle interception markers adjacent to dynamic path segments', () => { @@ -109,3 +110,40 @@ describe('getNamedRouteRegex', () => { expect(regex.re.test('/photos')).toBe(true) }) }) + +describe('parseParameter', () => { + it('should parse a optional catchall parameter', () => { + const param = '[[...slug]]' + const expected = { key: 'slug', repeat: true, optional: true } + const result = parseParameter(param) + expect(result).toEqual(expected) + }) + + it('should parse a catchall parameter', () => { + const param = '[...slug]' + const expected = { key: 'slug', repeat: true, optional: false } + const result = parseParameter(param) + expect(result).toEqual(expected) + }) + + it('should parse a optional parameter', () => { + const param = '[[foo]]' + const expected = { key: 'foo', repeat: false, optional: true } + const result = parseParameter(param) + expect(result).toEqual(expected) + }) + + it('should parse a dynamic parameter', () => { + const param = '[bar]' + const expected = { key: 'bar', repeat: false, optional: false } + const result = parseParameter(param) + expect(result).toEqual(expected) + }) + + it('should parse non-dynamic parameter', () => { + const param = 'fizz' + const expected = { key: 'fizz', repeat: false, optional: false } + const result = parseParameter(param) + expect(result).toEqual(expected) + }) +}) 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 f46c94e112dd0..57422b151609b 100644 --- a/packages/next/src/shared/lib/router/utils/route-regex.ts +++ b/packages/next/src/shared/lib/router/utils/route-regex.ts @@ -17,15 +17,51 @@ export interface RouteRegex { re: RegExp } +/** + * Regular expression pattern used to match route parameters. + * Matches both single parameters and parameter groups. + * Examples: + * - `[[...slug]]` matches parameter group with key 'slug', repeat: true, optional: true + * - `[...slug]` matches parameter group with key 'slug', repeat: true, optional: false + * - `[[foo]]` matches parameter with key 'foo', repeat: false, optional: true + * - `[bar]` matches parameter with key 'bar', repeat: false, optional: false + */ +const PARAMETER_PATTERN = /\[((?:\[.*\])|.+)\]/ + /** * Parses a given parameter from a route to a data structure that can be used - * to generate the parametrized route. Examples: + * to generate the parametrized route. + * Examples: + * - `[[...slug]]` -> `{ key: 'slug', repeat: true, optional: true }` + * - `[...slug]` -> `{ key: 'slug', repeat: true, optional: false }` + * - `[[foo]]` -> `{ key: 'foo', repeat: false, optional: true }` + * - `[bar]` -> `{ key: 'bar', repeat: false, optional: false }` + * - `fizz` -> `{ key: 'fizz', repeat: false, optional: false }` + * @param param - The parameter to parse. + * @returns The parsed parameter as a data structure. + */ +export function parseParameter(param: string) { + const match = param.match(PARAMETER_PATTERN) + + if (!match) { + return parseMatchedParameter(param) + } + + return parseMatchedParameter(match[1]) +} + +/** + * Parses a matched parameter from the PARAMETER_PATTERN regex to a data structure that can be used + * to generate the parametrized route. + * Examples: * - `[...slug]` -> `{ key: 'slug', repeat: true, optional: true }` * - `...slug` -> `{ key: 'slug', repeat: true, optional: false }` * - `[foo]` -> `{ key: 'foo', repeat: false, optional: true }` * - `bar` -> `{ key: 'bar', repeat: false, optional: false }` + * @param param - The matched parameter to parse. + * @returns The parsed parameter as a data structure. */ -export function parseParameter(param: string) { +function parseMatchedParameter(param: string) { const optional = param.startsWith('[') && param.endsWith(']') if (optional) { param = param.slice(1, -1) @@ -47,14 +83,18 @@ function getParametrizedRoute(route: string) { const markerMatch = INTERCEPTION_ROUTE_MARKERS.find((m) => segment.startsWith(m) ) - const paramMatches = segment.match(/\[((?:\[.*\])|.+)\]/) // Check for parameters + const paramMatches = segment.match(PARAMETER_PATTERN) // Check for parameters if (markerMatch && paramMatches) { - const { key, optional, repeat } = parseParameter(paramMatches[1]) + const { key, optional, repeat } = parseMatchedParameter( + paramMatches[1] + ) groups[key] = { pos: groupIndex++, repeat, optional } return `/${escapeStringRegexp(markerMatch)}([^/]+?)` } else if (paramMatches) { - const { key, repeat, optional } = parseParameter(paramMatches[1]) + const { key, repeat, optional } = parseMatchedParameter( + paramMatches[1] + ) groups[key] = { pos: groupIndex++, repeat, optional } return repeat ? (optional ? '(?:/(.+?))?' : '/(.+?)') : '/([^/]+?)' } else { @@ -110,7 +150,7 @@ function getSafeKeyFromSegment({ routeKeys: Record keyPrefix?: string }) { - const { key, optional, repeat } = parseParameter(segment) + const { key, optional, repeat } = parseMatchedParameter(segment) // replace any non-word characters since they can break // the named regex diff --git a/test/e2e/app-dir/parallel-routes-breadcrumbs/app/buzz/[[...fizz]]/page.tsx b/test/e2e/app-dir/parallel-routes-breadcrumbs/app/buzz/[[...fizz]]/page.tsx new file mode 100644 index 0000000000000..041f83d874df9 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-breadcrumbs/app/buzz/[[...fizz]]/page.tsx @@ -0,0 +1,7 @@ +export default function Page() { + return ( +
+

/buzz/[[...fizz]] Page!

+
+ ) +} diff --git a/test/e2e/app-dir/parallel-routes-breadcrumbs/app/fizz/[...buzz]/page.tsx b/test/e2e/app-dir/parallel-routes-breadcrumbs/app/fizz/[...buzz]/page.tsx new file mode 100644 index 0000000000000..374072101f8a0 --- /dev/null +++ b/test/e2e/app-dir/parallel-routes-breadcrumbs/app/fizz/[...buzz]/page.tsx @@ -0,0 +1,7 @@ +export default function Page() { + return ( +
+

/fizz/[...buzz] Page!

+
+ ) +} 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 index 63c35624b21cd..af7e57eed3cb6 100644 --- 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 @@ -1,4 +1,4 @@ -export default function StaticPage() { +export default function Page() { 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 e1e09ab7f1825..85dc9c350e224 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 @@ -59,4 +59,34 @@ describe('parallel-routes-breadcrumbs', () => { expect(await slot.text()).toContain('Album: en') expect(await slot.text()).toContain('Track: bar') }) + + it('should render the breadcrumbs correctly with catchall route segments', async () => { + const browser = await next.browser('/fizz/a/b') + const slot = await browser.waitForElementByCss('#slot') + + expect(await browser.elementByCss('h1').text()).toBe('Parallel Route!') + expect(await browser.elementByCss('h2').text()).toBe( + '/fizz/[...buzz] Page!' + ) + + // verify slot is rendering the params + expect(await slot.text()).toContain('Artist: fizz') + expect(await slot.text()).toContain('Album: a') + expect(await slot.text()).toContain('Track: b') + }) + + it('should render the breadcrumbs correctly with optional catchall route segments', async () => { + const browser = await next.browser('/buzz/a/b') + const slot = await browser.waitForElementByCss('#slot') + + expect(await browser.elementByCss('h1').text()).toBe('Parallel Route!') + expect(await browser.elementByCss('h2').text()).toBe( + '/buzz/[[...fizz]] Page!' + ) + + // verify slot is rendering the params + expect(await slot.text()).toContain('Artist: buzz') + expect(await slot.text()).toContain('Album: a') + expect(await slot.text()).toContain('Track: b') + }) }) From 2a2a4e7c0560f72a2578624e98264249e83e1d03 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Thu, 2 May 2024 20:49:11 +0200 Subject: [PATCH 2/2] Remove unnecessary mocks (#65284) Closes NEXT-3285 --- test/unit/next-image-new.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/unit/next-image-new.test.ts b/test/unit/next-image-new.test.ts index 60f8ea809c6a9..42f28c5fed720 100644 --- a/test/unit/next-image-new.test.ts +++ b/test/unit/next-image-new.test.ts @@ -4,10 +4,6 @@ import ReactDOMServer from 'react-dom/server' import Image from 'next/image' import cheerio from 'cheerio' -// Since this unit test doesn't check the result of -// ReactDOM.preload(), we can turn it into a noop. -jest.mock('react-dom', () => ({ preload: () => null })) - describe('Image rendering', () => { it('should render Image on its own', async () => { const element = React.createElement(Image, {