From 3869ee95ab268460285a0e49544172a3dc4662f7 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 4 Jul 2025 18:15:57 -0400 Subject: [PATCH] refactor(router): use getRouteParamKeys for consistent parameter ordering This refactor introduces a new getRouteParamKeys utility function that ensures route parameters are always accessed in the correct order based on their position in the route pattern. Previously, Object.keys() was used to iterate over route parameters, which could lead to inconsistent ordering since JavaScript object key order is not guaranteed for numeric string keys. Changes: - Add getRouteParamKeys utility that sorts parameters by their position - Replace Object.keys(groups) with getRouteParamKeys(groups) in: - server-utils.ts (normalizeDynamicRouteParams, interpolateDynamicPath, filterInternalQuery) - base-server.ts (normalizeCdnUrl parameter extraction) - Update omit() utility to accept readonly arrays for better type safety - Add comprehensive tests for getRouteParamKeys This ensures deterministic behavior when processing route parameters, which is critical for proper URL interpolation and parameter extraction. --- packages/next/src/build/static-paths/app.ts | 4 ++-- packages/next/src/build/static-paths/pages.ts | 3 ++- packages/next/src/server/base-server.ts | 5 ++++- .../src/server/request/fallback-params.ts | 13 +++-------- packages/next/src/server/server-utils.ts | 11 +++++----- packages/next/src/server/web-server.ts | 3 ++- .../next/src/shared/lib/router/adapters.tsx | 3 ++- packages/next/src/shared/lib/router/router.ts | 3 ++- .../shared/lib/router/utils/interpolate-as.ts | 6 ++--- .../next/src/shared/lib/router/utils/omit.ts | 2 +- .../lib/router/utils/route-param-keys.test.ts | 22 +++++++++++++++++++ .../lib/router/utils/route-param-keys.ts | 19 ++++++++++++++++ 12 files changed, 68 insertions(+), 26 deletions(-) create mode 100644 packages/next/src/shared/lib/router/utils/route-param-keys.test.ts create mode 100644 packages/next/src/shared/lib/router/utils/route-param-keys.ts diff --git a/packages/next/src/build/static-paths/app.ts b/packages/next/src/build/static-paths/app.ts index 4f1fce57dd0e4..d5e1855d2b776 100644 --- a/packages/next/src/build/static-paths/app.ts +++ b/packages/next/src/build/static-paths/app.ts @@ -7,7 +7,7 @@ import path from 'node:path' import { AfterRunner } from '../../server/after/run-with-after' import { createWorkStore } from '../../server/async-storage/work-store' import { FallbackMode } from '../../lib/fallback' -import { getRouteMatcher } from '../../shared/lib/router/utils/route-matcher' +import { getRouteParamKeys } from '../../shared/lib/router/utils/route-param-keys' import { getRouteRegex, type RouteRegex, @@ -471,7 +471,7 @@ export async function buildAppStaticPaths({ }) const regex = getRouteRegex(page) - const routeParamKeys = Object.keys(getRouteMatcher(regex)(page) || {}) + const routeParamKeys = getRouteParamKeys(regex.groups) const afterRunner = new AfterRunner() diff --git a/packages/next/src/build/static-paths/pages.ts b/packages/next/src/build/static-paths/pages.ts index b2f7358a1e3e7..964cc52bd8191 100644 --- a/packages/next/src/build/static-paths/pages.ts +++ b/packages/next/src/build/static-paths/pages.ts @@ -8,6 +8,7 @@ import { removeTrailingSlash } from '../../shared/lib/router/utils/remove-traili import { getRouteMatcher } from '../../shared/lib/router/utils/route-matcher' import { getRouteRegex } from '../../shared/lib/router/utils/route-regex' import { encodeParam, normalizePathname } from './utils' +import { getRouteParamKeys } from '../../shared/lib/router/utils/route-param-keys' export async function buildPagesStaticPaths({ page, @@ -27,7 +28,7 @@ export async function buildPagesStaticPaths({ const _routeMatcher = getRouteMatcher(_routeRegex) // Get the default list of allowed params. - const routeParameterKeys = Object.keys(_routeMatcher(page)) + const routeParameterKeys = getRouteParamKeys(_routeRegex.groups) const staticPathsResult = await getStaticPaths({ // We create a copy here to avoid having the types of `getStaticPaths` // change. This ensures that users can't mutate this array and have it diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 5611478f0caef..20502a319b664 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -85,6 +85,7 @@ import { denormalizePagePath } from '../shared/lib/page-path/denormalize-page-pa import * as Log from '../build/output/log' import { getPreviouslyRevalidatedTags, getServerUtils } from './server-utils' import isError, { getProperError } from '../lib/is-error' +import { getRouteParamKeys } from '../shared/lib/router/utils/route-param-keys' import { addRequestMeta, getRequestMeta, @@ -1396,7 +1397,9 @@ export default abstract class Server< if (pageIsDynamic || didRewrite) { utils.normalizeCdnUrl(req, [ ...rewriteParamKeys, - ...Object.keys(utils.defaultRouteRegex?.groups || {}), + ...(utils.defaultRouteRegex + ? getRouteParamKeys(utils.defaultRouteRegex.groups) + : []), ]) } // Remove the route `params` keys from `parsedUrl.query` if they are diff --git a/packages/next/src/server/request/fallback-params.ts b/packages/next/src/server/request/fallback-params.ts index 52ebe2c2f7f65..1bd5f11d1ebb7 100644 --- a/packages/next/src/server/request/fallback-params.ts +++ b/packages/next/src/server/request/fallback-params.ts @@ -1,22 +1,15 @@ -import { getRouteMatcher } from '../../shared/lib/router/utils/route-matcher' +import { getRouteParamKeys } from '../../shared/lib/router/utils/route-param-keys' import { getRouteRegex } from '../../shared/lib/router/utils/route-regex' export type FallbackRouteParams = ReadonlyMap -function getParamKeys(page: string) { - const pattern = getRouteRegex(page) - const matcher = getRouteMatcher(pattern) - - // Get the default list of allowed params. - return Object.keys(matcher(page)) -} - export function getFallbackRouteParams( pageOrKeys: string | readonly string[] ): FallbackRouteParams | null { let keys: readonly string[] if (typeof pageOrKeys === 'string') { - keys = getParamKeys(pageOrKeys) + const { groups } = getRouteRegex(pageOrKeys) + keys = getRouteParamKeys(groups) } else { keys = pageOrKeys } diff --git a/packages/next/src/server/server-utils.ts b/packages/next/src/server/server-utils.ts index 60f9b937e7f6e..9c8a1a07f2d8d 100644 --- a/packages/next/src/server/server-utils.ts +++ b/packages/next/src/server/server-utils.ts @@ -31,10 +31,11 @@ import { parseAndValidateFlightRouterState } from './app-render/parse-and-valida import { isInterceptionRouteRewrite } from '../lib/generate-interception-routes-rewrites' import { NEXT_ROUTER_STATE_TREE_HEADER } from '../client/components/app-router-headers' import { getSelectedParams } from '../client/components/router-reducer/compute-changed-path' +import { getRouteParamKeys } from '../shared/lib/router/utils/route-param-keys' function filterInternalQuery( query: Record, - paramKeys: string[] + paramKeys: readonly string[] ) { // this is used to pass query information in rewrites // but should not be exposed in final query @@ -60,7 +61,7 @@ function filterInternalQuery( export function normalizeCdnUrl( req: BaseNextRequest | IncomingMessage, - paramKeys: string[] + paramKeys: readonly string[] ) { // make sure to normalize req.url from CDNs to strip dynamic and rewrite // params from the query which are added during routing @@ -83,7 +84,7 @@ export function interpolateDynamicPath( ) { if (!defaultRouteRegex) return pathname - for (const param of Object.keys(defaultRouteRegex.groups)) { + for (const param of getRouteParamKeys(defaultRouteRegex.groups)) { const { optional, repeat } = defaultRouteRegex.groups[param] let builtParam = `[${repeat ? '...' : ''}${param}]` @@ -119,7 +120,7 @@ export function normalizeDynamicRouteParams( let hasValidParams = true let params: ParsedUrlQuery = {} - for (const key of Object.keys(defaultRouteRegex.groups)) { + for (const key of getRouteParamKeys(defaultRouteRegex.groups)) { let value: string | string[] | undefined = query[key] if (typeof value === 'string') { @@ -396,7 +397,7 @@ export function getServerUtils({ // Use all the named route keys. const result = {} as RegExpExecArray - for (const keyName of Object.keys(routeKeys)) { + for (const keyName of getRouteParamKeys(groups)) { const paramName = routeKeys[keyName] // If this param name is not a valid parameter name, then skip it. diff --git a/packages/next/src/server/web-server.ts b/packages/next/src/server/web-server.ts index 5791868170cfa..cbf5c6cf127bf 100644 --- a/packages/next/src/server/web-server.ts +++ b/packages/next/src/server/web-server.ts @@ -35,6 +35,7 @@ import { getEdgeInstrumentationModule } from './web/globals' import type { ServerOnInstrumentationRequestError } from './app-render/types' import { getEdgePreviewProps } from './web/get-edge-preview-props' import { NoFallbackError } from '../shared/lib/no-fallback-error.external' +import { getRouteParamKeys } from '../shared/lib/router/utils/route-param-keys' interface WebServerOptions extends Options { buildId: string @@ -186,7 +187,7 @@ export default class NextWebServer extends BaseServer< normalizedParams, routeRegex ) - normalizeCdnUrl(req, Object.keys(routeRegex.routeKeys)) + normalizeCdnUrl(req, getRouteParamKeys(routeRegex.groups)) } } diff --git a/packages/next/src/shared/lib/router/adapters.tsx b/packages/next/src/shared/lib/router/adapters.tsx index cc3e34b3dfd04..2489574bc9c1a 100644 --- a/packages/next/src/shared/lib/router/adapters.tsx +++ b/packages/next/src/shared/lib/router/adapters.tsx @@ -7,6 +7,7 @@ import { PathnameContext } from '../hooks-client-context.shared-runtime' import { isDynamicRoute } from './utils' import { asPathToSearchParams } from './utils/as-path-to-search-params' import { getRouteRegex } from './utils/route-regex' +import { getRouteParamKeys } from './utils/route-param-keys' /** It adapts a Pages Router (`NextRouter`) to the App Router Instance. */ export function adaptForAppRouterInstance( @@ -59,7 +60,7 @@ export function adaptForPathParams( } const pathParams: Params = {} const routeRegex = getRouteRegex(router.pathname) - const keys = Object.keys(routeRegex.groups) + const keys = getRouteParamKeys(routeRegex.groups) for (const key of keys) { pathParams[key] = router.query[key]! } diff --git a/packages/next/src/shared/lib/router/router.ts b/packages/next/src/shared/lib/router/router.ts index f799067b70917..62660eb38970a 100644 --- a/packages/next/src/shared/lib/router/router.ts +++ b/packages/next/src/shared/lib/router/router.ts @@ -24,6 +24,7 @@ import { isDynamicRoute } from './utils/is-dynamic' import { parseRelativeUrl } from './utils/parse-relative-url' import { getRouteMatcher } from './utils/route-matcher' import { getRouteRegex } from './utils/route-regex' +import { getRouteParamKeys } from './utils/route-param-keys' import { formatWithValidation } from './utils/format-url' import { detectDomainLocale } from '../../../client/detect-domain-locale' import { parsePath } from './utils/parse-path' @@ -1504,7 +1505,7 @@ export default class Router implements BaseRouter { : ({} as { result: undefined; params: undefined }) if (!routeMatch || (shouldInterpolate && !interpolatedAs.result)) { - const missingParams = Object.keys(routeRegex.groups).filter( + const missingParams = getRouteParamKeys(routeRegex.groups).filter( (param) => !query[param] && !routeRegex.groups[param].optional ) diff --git a/packages/next/src/shared/lib/router/utils/interpolate-as.ts b/packages/next/src/shared/lib/router/utils/interpolate-as.ts index 6035a8d16f0ff..dc0406f7d285d 100644 --- a/packages/next/src/shared/lib/router/utils/interpolate-as.ts +++ b/packages/next/src/shared/lib/router/utils/interpolate-as.ts @@ -2,6 +2,7 @@ import type { ParsedUrlQuery } from 'querystring' import { getRouteMatcher } from './route-matcher' import { getRouteRegex } from './route-regex' +import { getRouteParamKeys } from './route-param-keys' export function interpolateAs( route: string, @@ -11,7 +12,6 @@ export function interpolateAs( let interpolatedRoute = '' const dynamicRegex = getRouteRegex(route) - const dynamicGroups = dynamicRegex.groups const dynamicMatches = // Try to match the dynamic route against the asPath (asPathname !== route ? getRouteMatcher(dynamicRegex)(asPathname) : '') || @@ -20,12 +20,12 @@ export function interpolateAs( query interpolatedRoute = route - const params = Object.keys(dynamicGroups) + const params = getRouteParamKeys(dynamicRegex.groups) if ( !params.every((param) => { let value = dynamicMatches[param] || '' - const { repeat, optional } = dynamicGroups[param] + const { repeat, optional } = dynamicRegex.groups[param] // support single-level catch-all // TODO: more robust handling for user-error (passing `/`) diff --git a/packages/next/src/shared/lib/router/utils/omit.ts b/packages/next/src/shared/lib/router/utils/omit.ts index 7516157ec170e..a5e0507b1b914 100644 --- a/packages/next/src/shared/lib/router/utils/omit.ts +++ b/packages/next/src/shared/lib/router/utils/omit.ts @@ -1,6 +1,6 @@ export function omit( object: T, - keys: K[] + keys: readonly K[] ): Omit { const omitted: { [key: string]: unknown } = {} Object.keys(object).forEach((key) => { diff --git a/packages/next/src/shared/lib/router/utils/route-param-keys.test.ts b/packages/next/src/shared/lib/router/utils/route-param-keys.test.ts new file mode 100644 index 0000000000000..2b8fc8960ffe5 --- /dev/null +++ b/packages/next/src/shared/lib/router/utils/route-param-keys.test.ts @@ -0,0 +1,22 @@ +import { getRouteRegex } from './route-regex' +import { getRouteParamKeys } from './route-param-keys' + +describe('getRouteParamKeys', () => { + it('should return the correct param keys', () => { + const { groups } = getRouteRegex('/[...slug].json') + expect(getRouteParamKeys(groups)).toEqual(['slug']) + }) + + it('should have the correct ordering', () => { + const { groups } = getRouteRegex('/[lang]/[...slug]') + expect(getRouteParamKeys(groups)).toEqual(['lang', 'slug']) + }) + + it('should have the correct ordering when the groups object is not sorted', () => { + const groups = { + slug: { pos: 2, repeat: true, optional: false }, + lang: { pos: 1, repeat: false, optional: false }, + } + expect(getRouteParamKeys(groups)).toEqual(['lang', 'slug']) + }) +}) diff --git a/packages/next/src/shared/lib/router/utils/route-param-keys.ts b/packages/next/src/shared/lib/router/utils/route-param-keys.ts new file mode 100644 index 0000000000000..18a75df0073f7 --- /dev/null +++ b/packages/next/src/shared/lib/router/utils/route-param-keys.ts @@ -0,0 +1,19 @@ +import type { Group } from './route-regex' + +/** + * Get the parameter keys from a route regex, sorted by their position in the + * route regex which corresponds to the order they appear in the route. + * + * @param groups - The groups of the route regex. + * @returns The parameter keys in the order they appear in the route regex. + */ +export function getRouteParamKeys( + groups: Record +): readonly string[] { + const keys = Object.keys(groups) + + // Sort keys directly by their position values + keys.sort((a, b) => groups[a].pos - groups[b].pos) + + return keys +}