Skip to content

Commit

Permalink
Provide non-dynamic segments to catch-all parallel routes (#65233)
Browse files Browse the repository at this point in the history
Fixes an issue with #65063 where
the catch-all param only contains dynamic segments and is missing
non-dynamic route segments.

This makes building breadcrumbs extremely hard as we're missing
information to properly render the correct breadcrumb components.

This fix makes parallel route catch-all params behave like the standard
catch-all params in non-parallel routes

---------

Co-authored-by: Zack Tanner <1939140+ztanner@users.noreply.github.com>
  • Loading branch information
agadzik and ztanner authored May 1, 2024
1 parent f2e7c8e commit bb7f5a3
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 17 deletions.
53 changes: 38 additions & 15 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ import {
} from '../client-component-renderer-logger'
import { createServerModuleMap } from './action-utils'
import { isNodeNextRequest } from '../base-http/helpers'
import { parseParameter } from '../../shared/lib/router/utils/route-regex'

export type GetDynamicParamFromSegment = (
// [slug] / [[slug]] / [...slug]
Expand Down Expand Up @@ -207,6 +208,7 @@ export type CreateSegmentPath = (child: FlightSegmentPath) => FlightSegmentPath
*/
function makeGetDynamicParamFromSegment(
params: { [key: string]: any },
pagePath: string,
flightRouterState: FlightRouterState | undefined
): GetDynamicParamFromSegment {
return function getDynamicParamFromSegment(
Expand Down Expand Up @@ -234,26 +236,46 @@ 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'
) {
// 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))
const hasValues = value.length > 0
const type = dynamicParamTypes[segmentParam.type]
const isCatchall = segmentParam.type === 'catchall'
const isOptionalCatchall = segmentParam.type === 'optional-catchall'

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
value = pagePath
.split('/')
// remove the first empty string
.slice(1)
// replace any dynamic params with the actual values
.map((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
})

return {
param: key,
value: hasValues ? value : null,
type: type,
value,
type: dynamicParamType,
// This value always has to be a string.
treeSegment: [key, hasValues ? value.join('/') : '', type],
treeSegment: [key, value.join('/'), dynamicParamType],
}
}

return findDynamicParamFromRouterState(flightRouterState, segment)
}

Expand Down Expand Up @@ -795,6 +817,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
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/shared/lib/router/utils/route-regex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export default function Page({ params: { catchAll } }) {
export default function Page({ params: { catchAll = [] } }) {
return (
<div id="slot">
<h1>Parallel Route!</h1>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export default function StaticPage() {
return (
<div>
<h2>/foo/[lang]/bar Page!</h2>
</div>
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
})

0 comments on commit bb7f5a3

Please sign in to comment.