Skip to content

Commit

Permalink
Revert "fix: handle optional catchalls correctly" (vercel#75442)
Browse files Browse the repository at this point in the history 
We noticed a routing regression, so reverting while we investigate. 

Reverts vercel#75377
  • Loading branch information
@ztanner
ztanner authored Jan 29, 2025
1 parent c0d23ae commit 8514cd6
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 170 deletions.
12 changes: 8 additions & 4 deletions packages/next/src/server/base-server.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1237,13 +1237,16 @@ export default abstract class Server<
typeof routeMatchesHeader === 'string' &&
routeMatchesHeader &&
isDynamicRoute(matchedPath) &&
(Object.keys(params).length === 0 || !paramsResult.hasValidParams)
!paramsResult.hasValidParams
) {
const routeMatches =
utils.getParamsFromRouteMatches(routeMatchesHeader)

if (routeMatches) {
paramsResult = utils.normalizeDynamicRouteParams(routeMatches)
paramsResult = utils.normalizeDynamicRouteParams(
routeMatches,
true
)

if (paramsResult.hasValidParams) {
params = paramsResult.params
Expand All @@ -1256,7 +1259,8 @@ export default abstract class Server<
utils.defaultRouteMatches &&
normalizedUrlPath === srcPathname &&
!paramsResult.hasValidParams &&
!utils.normalizeDynamicRouteParams({ ...params }).hasValidParams
!utils.normalizeDynamicRouteParams({ ...params }, true)
.hasValidParams
) {
params = utils.defaultRouteMatches

Expand All @@ -1272,7 +1276,7 @@ export default abstract class Server<
}

if (pageIsDynamic || didRewrite) {
utils.normalizeVercelUrl(req, [
utils.normalizeVercelUrl(req, true, [
...rewriteParamKeys,
...Object.keys(utils.defaultRouteRegex?.groups || {}),
])
Expand Down
89 changes: 53 additions & 36 deletions packages/next/src/server/server-utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,34 +24,36 @@ import { normalizeNextQueryParam } from './web/utils'

export function normalizeVercelUrl(
req: BaseNextRequest,
paramKeys: string[],
defaultRouteRegex: ReturnType<typeof getNamedRouteRegex> | undefined
trustQuery: boolean,
paramKeys?: string[],
pageIsDynamic?: boolean,
defaultRouteRegex?: ReturnType<typeof getNamedRouteRegex> | undefined
) {
if (!defaultRouteRegex) return

// make sure to normalize req.url on Vercel to strip dynamic params
// from the query which are added during routing
const _parsedUrl = parseUrl(req.url!, true)
delete (_parsedUrl as any).search

for (const key of Object.keys(_parsedUrl.query)) {
const isNextQueryPrefix =
key !== NEXT_QUERY_PARAM_PREFIX && key.startsWith(NEXT_QUERY_PARAM_PREFIX)

const isNextInterceptionMarkerPrefix =
key !== NEXT_INTERCEPTION_MARKER_PREFIX &&
key.startsWith(NEXT_INTERCEPTION_MARKER_PREFIX)

if (
isNextQueryPrefix ||
isNextInterceptionMarkerPrefix ||
(paramKeys || Object.keys(defaultRouteRegex.groups)).includes(key)
) {
delete _parsedUrl.query[key]
if (pageIsDynamic && trustQuery && defaultRouteRegex) {
const _parsedUrl = parseUrl(req.url!, true)
delete (_parsedUrl as any).search

for (const key of Object.keys(_parsedUrl.query)) {
const isNextQueryPrefix =
key !== NEXT_QUERY_PARAM_PREFIX &&
key.startsWith(NEXT_QUERY_PARAM_PREFIX)

const isNextInterceptionMarkerPrefix =
key !== NEXT_INTERCEPTION_MARKER_PREFIX &&
key.startsWith(NEXT_INTERCEPTION_MARKER_PREFIX)

if (
isNextQueryPrefix ||
isNextInterceptionMarkerPrefix ||
(paramKeys || Object.keys(defaultRouteRegex.groups)).includes(key)
) {
delete _parsedUrl.query[key]
}
}
req.url = formatUrl(_parsedUrl)
}

req.url = formatUrl(_parsedUrl)
}

export function interpolateDynamicPath(
Expand Down Expand Up @@ -88,10 +90,13 @@ export function interpolateDynamicPath(

export function normalizeDynamicRouteParams(
params: ParsedUrlQuery,
defaultRouteRegex: ReturnType<typeof getNamedRouteRegex>,
defaultRouteMatches: ParsedUrlQuery
ignoreOptional?: boolean,
defaultRouteRegex?: ReturnType<typeof getNamedRouteRegex> | undefined,
defaultRouteMatches?: ParsedUrlQuery | undefined
) {
let hasValidParams = true
if (!defaultRouteRegex) return { params, hasValidParams: false }

params = Object.keys(defaultRouteRegex.groups).reduce((prev, key) => {
let value: string | string[] | undefined = params[key]

Expand Down Expand Up @@ -121,7 +126,10 @@ export function normalizeDynamicRouteParams(
})
: value?.includes(defaultValue as string)

if (isDefaultValue || (typeof value === 'undefined' && !isOptional)) {
if (
isDefaultValue ||
(typeof value === 'undefined' && !(isOptional && ignoreOptional))
) {
hasValidParams = false
}

Expand Down Expand Up @@ -367,19 +375,28 @@ export function getUtils({
dynamicRouteMatcher,
defaultRouteMatches,
getParamsFromRouteMatches,
normalizeDynamicRouteParams: (params: ParsedUrlQuery) => {
if (!defaultRouteRegex || !defaultRouteMatches) {
return { params, hasValidParams: false }
}

return normalizeDynamicRouteParams(
normalizeDynamicRouteParams: (
params: ParsedUrlQuery,
ignoreOptional?: boolean
) =>
normalizeDynamicRouteParams(
params,
ignoreOptional,
defaultRouteRegex,
defaultRouteMatches
)
},
normalizeVercelUrl: (req: BaseNextRequest, paramKeys: string[]) =>
normalizeVercelUrl(req, paramKeys, defaultRouteRegex),
),
normalizeVercelUrl: (
req: BaseNextRequest,
trustQuery: boolean,
paramKeys?: string[]
) =>
normalizeVercelUrl(
req,
trustQuery,
paramKeys,
pageIsDynamic,
defaultRouteRegex
),
interpolateDynamicPath: (
pathname: string,
params: Record<string, undefined | string | string[]>
Expand Down
9 changes: 8 additions & 1 deletion packages/next/src/server/web-server.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ export default class NextWebServer extends BaseServer<
) as NextParsedUrlQuery
const paramsResult = normalizeDynamicRouteParams(
query,
false,
routeRegex,
defaultRouteMatches
)
Expand All @@ -185,7 +186,13 @@ export default class NextWebServer extends BaseServer<
normalizedParams,
routeRegex
)
normalizeVercelUrl(req, Object.keys(routeRegex.routeKeys), routeRegex)
normalizeVercelUrl(
req,
true,
Object.keys(routeRegex.routeKeys),
true,
routeRegex
)
}
}

Expand Down
29 changes: 0 additions & 29 deletions ...integration/required-server-files-ssr-404/pages/partial-catch-all/[domain]/[[...rest]].js
View file

This file was deleted.

96 changes: 2 additions & 94 deletions test/integration/required-server-files-ssr-404/test/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ describe('Required Server Files', () => {
{
headers: {
'x-matched-path': '/catch-all/[[...rest]]',
'x-now-route-matches': 'nxtPrest=hello',
'x-now-route-matches': 'nxtPrest=hello&catchAll=hello',
},
}
)
Expand All @@ -342,7 +342,7 @@ describe('Required Server Files', () => {

const html3 = await renderViaHTTP(
appPort,
'/catch-all/[[...rest]]',
'/catch-all/[[..rest]]',
undefined,
{
headers: {
Expand All @@ -359,98 +359,6 @@ describe('Required Server Files', () => {
expect(data3.params).toEqual({ rest: ['hello', 'world'] })
expect(isNaN(data3.random)).toBe(false)
expect(data3.random).not.toBe(data.random)

const html4 = await renderViaHTTP(
appPort,
'/catch-all/[[...rest]]',
{ nxtPrest: 'frank' },
{
headers: {
'x-matched-path': '/catch-all/[[...rest]]',
},
}
)
const $4 = cheerio.load(html4)
const data4 = JSON.parse($4('#props').text())

expect($4('#catch-all').text()).toBe('optional catch-all page')
expect(data4.params).toEqual({ rest: ['frank'] })
expect(isNaN(data4.random)).toBe(false)
expect(data4.random).not.toBe(data.random)

const html5 = await renderViaHTTP(
appPort,
'/catch-all/[[...rest]]',
{},
{
headers: {
'x-matched-path': '/catch-all/[[...rest]]',
},
}
)
const $5 = cheerio.load(html5)
const data5 = JSON.parse($5('#props').text())

expect($5('#catch-all').text()).toBe('optional catch-all page')
expect(data5.params).toEqual({})
expect(isNaN(data5.random)).toBe(false)
expect(data5.random).not.toBe(data.random)

const html6 = await renderViaHTTP(
appPort,
'/catch-all/[[...rest]]',
{ nxtPrest: 'frank' },
{
headers: {
'x-matched-path': '/catch-all/[[...rest]]',
},
}
)
const $6 = cheerio.load(html6)
const data6 = JSON.parse($6('#props').text())

expect($6('#catch-all').text()).toBe('optional catch-all page')
expect(data6.params).toEqual({ rest: ['frank'] })
expect(isNaN(data6.random)).toBe(false)
expect(data6.random).not.toBe(data.random)
})

describe('partial optional catch-all route', () => {
it.each([
{
path: '/partial-catch-all/hello.com',
query: { nxtPdomain: 'hello.com' },
expected: { domain: 'hello.com' },
},
{
path: '/partial-catch-all/hello.com/hello',
query: { nxtPdomain: 'hello.com', nxtPrest: 'hello' },
expected: { domain: 'hello.com', rest: ['hello'] },
},
{
path: '/partial-catch-all/hello.com/hello/world',
query: { nxtPdomain: 'hello.com', nxtPrest: 'hello/world' },
expected: { domain: 'hello.com', rest: ['hello', 'world'] },
},
])('should render $path', async ({ query, expected }) => {
const html = await renderViaHTTP(
appPort,
'/partial-catch-all/[domain]/[[...rest]]',
query,
{
headers: {
'x-matched-path': '/partial-catch-all/[domain]/[[...rest]]',
},
}
)

const $ = cheerio.load(html)
const data = JSON.parse($('#props').text())

expect($('#catch-all').text()).toBe('partial optional catch-all page')
expect(data.params).toEqual(expected)
expect(data.hello).toBe('world')
})
})

it('should return data correctly with x-matched-path for optional catch-all route', async () => {
Expand Down
3 changes: 0 additions & 3 deletions test/turbopack-build-tests-manifest.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14483,9 +14483,6 @@
"Required Server Files production mode should render dynamic SSR page correctly",
"Required Server Files production mode should render dynamic SSR page correctly with x-matched-path",
"Required Server Files production mode should render fallback optional catch-all route correctly with x-matched-path and routes-matches",
"Required Server Files production mode partial optional catch-all route should render /partial-catch-all/hello.com",
"Required Server Files production mode partial optional catch-all route should render /partial-catch-all/hello.com/hello",
"Required Server Files production mode partial optional catch-all route should render /partial-catch-all/hello.com/hello/world",
"Required Server Files production mode should render fallback page correctly",
"Required Server Files production mode should render fallback page correctly with x-matched-path and routes-matches",
"Required Server Files production mode should return data correctly with x-matched-path",
Expand Down
3 changes: 0 additions & 3 deletions test/turbopack-dev-tests-manifest.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18641,9 +18641,6 @@
"Required Server Files production mode should render dynamic SSR page correctly",
"Required Server Files production mode should render dynamic SSR page correctly with x-matched-path",
"Required Server Files production mode should render fallback optional catch-all route correctly with x-matched-path and routes-matches",
"Required Server Files production mode partial optional catch-all route should render /partial-catch-all/hello.com",
"Required Server Files production mode partial optional catch-all route should render /partial-catch-all/hello.com/hello",
"Required Server Files production mode partial optional catch-all route should render /partial-catch-all/hello.com/hello/world",
"Required Server Files production mode should render fallback page correctly",
"Required Server Files production mode should render fallback page correctly with x-matched-path and routes-matches",
"Required Server Files production mode should return data correctly with x-matched-path",
Expand Down

0 comments on commit 8514cd6

Please sign in to comment.