From 8688e6030e3d22ce5c65351908996ee4a284484a Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Tue, 29 Nov 2022 17:21:26 -0500 Subject: [PATCH 1/6] refactor(router): handle optional path segments in nested routes explicitly 'explode' path with optional path segments. --- .../__tests__/path-matching-test.tsx | 59 +++++++- packages/router/utils.ts | 126 +++++++++++++----- 2 files changed, 147 insertions(+), 38 deletions(-) diff --git a/packages/react-router/__tests__/path-matching-test.tsx b/packages/react-router/__tests__/path-matching-test.tsx index 4f4a4ad32e..6d64b180b5 100644 --- a/packages/react-router/__tests__/path-matching-test.tsx +++ b/packages/react-router/__tests__/path-matching-test.tsx @@ -10,7 +10,20 @@ function pickPathsAndParams(routes: RouteObject[], pathname: string) { let matches = matchRoutes(routes, pathname); return ( matches && - matches.map((match) => ({ path: match.route.path, params: match.params })) + matches.map((match) => { + let { path } = match.route + + // filter params to only include params for the current (relative) route path + let paramNames = path?.split('/') + .filter(seg => seg.startsWith(':')) + .map(paramSeg => paramSeg.replace(/^:/, "").replace(/\?$/, "")) + let params = Object.fromEntries(Object.entries(match.params).filter(([k,v]) => paramNames?.includes(k))) + + return { + path: match.route.path, + params, + } + }) ); } @@ -277,7 +290,7 @@ describe("path matching with splats", () => { }); }); -describe("path matchine with optional segments", () => { +describe("path matching with optional segments", () => { test("optional static segment at the start of the path", () => { let routes = [ { @@ -357,6 +370,25 @@ describe("path matchine with optional segments", () => { }, ]); }); + + test("optional static segment in nested routes", () => { + let nested = [ + { + path: "/en?", + children: [ + { + path: "abc", + }, + ], + }, + ]; + + + expect(pickPathsAndParams(nested, "/en/abc")).toEqual([ + { path: "/en?", params: {} }, + { path: "abc", params: {} }, + ]); + }); }); describe("path matching with optional dynamic segments", () => { @@ -439,4 +471,27 @@ describe("path matching with optional dynamic segments", () => { }, ]); }); + + test("consecutive optional dynamic segments in nested routes", () => { + let nested = [ + { + path: "/one/:two?", + children: [ + { + path: "three/:four?", + children: [ + { + path: ":five?" + } + ] + } + ], + }, + ]; + expect(pickPathsAndParams(nested, "/one/three/cuatro/cinco")).toEqual([ + {path: "/one/:two?", params: {}}, + {path: "three/:four?", params: { four: 'cuatro' }}, + {path: ":five?", params: { five: 'cinco' }}, + ]) + }); }); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 72699b4ee1..b19e65c033 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -372,9 +372,13 @@ function flattenRoutes< parentsMeta: RouteMeta[] = [], parentPath = "" ): RouteBranch[] { - routes.forEach((route, index) => { + let flattenRoute = ( + route: RouteObjectType, + index: number, + relativePath?: string + ) => { let meta: RouteMeta = { - relativePath: route.path || "", + relativePath: relativePath || route.path || "", caseSensitive: route.caseSensitive === true, childrenIndex: index, route, @@ -415,48 +419,98 @@ function flattenRoutes< return; } - // Handle optional params - /path/:optional? - let segments = path.split("/"); - let optionalParams: string[] = []; - segments.forEach((segment) => { - let match = segment.match(/^:?([^?]+)\?$/); - if (match) { - optionalParams.push(match[1]); - } + branches.push({ + path, + score: computeScore(path, route.index), + routesMeta, }); - - if (optionalParams.length > 0) { - for (let i = 0; i <= optionalParams.length; i++) { - let newPath = path; - let newMeta = routesMeta.map((m) => ({ ...m })); - - for (let j = optionalParams.length - 1; j >= 0; j--) { - let re = new RegExp(`(\\/:?${optionalParams[j]})\\?`); - let replacement = j < i ? "$1" : ""; - newPath = newPath.replace(re, replacement); - newMeta[newMeta.length - 1].relativePath = newMeta[ - newMeta.length - 1 - ].relativePath.replace(re, replacement); - } - - branches.push({ - path: newPath, - score: computeScore(newPath, route.index), - routesMeta: newMeta, - }); - } + }; + routes.forEach((route, index) => { + // coarse-grain check for optional params + if (route.path === "" || !route.path?.includes("?")) { + flattenRoute(route, index); } else { - branches.push({ - path, - score: computeScore(path, route.index), - routesMeta, - }); + for (let exploded of explodeOptionalSegments(route.path)) { + flattenRoute(route, index, exploded); + } } }); return branches; } +/** + * Computes all combinations of optional path segments for a given path. + */ +let _explodeOptionalSegments = (path: string): string[] => { + let segments = path.split("/"); + if (segments.length === 0) return []; + + let [first, ...rest] = segments; + + // Optional path segments are denoted by a trailing `?` + let isOptional = first.endsWith("?"); + // Compute the corresponding required segment: `foo?` -> `foo` + let required = first.replace(/\?$/, ""); + + if (rest.length === 0) { + // Intepret empty string as omitting an optional segment + // `["one", "", "three"]` corresponds to omitting `:two` from `/one/:two?/three` -> `/one/three` + return isOptional ? ["", required] : [required]; + } + + let restExploded = _explodeOptionalSegments(rest.join("/")); + return restExploded.flatMap((subpath) => { + // /one + / + :two/three -> /one/:two/three + let requiredExploded = subpath === "" ? required : required + "/" + subpath; + // For optional segments, return the exploded path _without_ current segment first (`subpath`) + // and exploded path _with_ current segment later (`subpath`) + // This ensures that exploded paths are emitted in priority order + // `/one/three/:four` will come before `/one/three/:five` + return isOptional ? [subpath, requiredExploded] : [requiredExploded]; + }); +}; + +/** + * Computes all combinations of optional path segments for a given path, + * excluding combinations that are ambiguous and of lower priority. + * + * For example, `/one/:two?/three/:four?/:five?` explodes to: + * - `/one/three` + * - `/one/:two/three` + * - `/one/three/:four` + * - `/one/:two/three/:four` + * - `/one/three/:four/:five` + * - `/one/:two/three/:four/:five` + * + * Note that these paths are not returned: + * - `/one/three/:five` (because `/one/three/:four` has priority) + * - `/one/:two/three/:five` (because `/one/:two/three/:four` has priority) + */ +let explodeOptionalSegments = function* (path: string) { + // Compute hash for dynamic path segments + // /one/:two/three/:four -> /one/:/three/: + let dynamicHash = (subpath: string) => + subpath + .split("/") + .map((segment) => (segment.startsWith(":") ? ":" : segment)) + .join("/"); + + let dynamicHashes = new Set(); + for (let exploded of _explodeOptionalSegments(path)) { + let hash = dynamicHash(exploded); + + // `/one/three/:four` and `/one/three/:five` have the same hash: `/one/three/:` + // so we only emit the first one of these we come across + // _explodeOptionalSegments returns exploded paths in priority order, + // so `/one/three/:four` will come before `/one/three/:five` + if (dynamicHashes.has(hash)) continue; + + dynamicHashes.add(hash); + yield exploded; + } +}; + function rankRouteBranches(branches: RouteBranch[]): void { branches.sort((a, b) => a.score !== b.score From 28836f2d553b306fa5fed91f1c372110cdc2f964 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Tue, 6 Dec 2022 20:53:41 -0500 Subject: [PATCH 2/6] fix: handle empty segments for nested routes --- packages/router/utils.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index b19e65c033..186351dbb1 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -378,7 +378,7 @@ function flattenRoutes< relativePath?: string ) => { let meta: RouteMeta = { - relativePath: relativePath || route.path || "", + relativePath: relativePath ?? (route.path || ""), caseSensitive: route.caseSensitive === true, childrenIndex: index, route, @@ -507,6 +507,13 @@ let explodeOptionalSegments = function* (path: string) { if (dynamicHashes.has(hash)) continue; dynamicHashes.add(hash); + + // for absolute paths, ensure `/` instead of empty segment + if (path.startsWith("/") && exploded === "") { + yield "/"; + continue; + } + yield exploded; } }; From e4cc3745e6da376885d63aea62cf814786f75c65 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Wed, 7 Dec 2022 12:32:34 -0500 Subject: [PATCH 3/6] refactor: replace generator with function returning a list --- packages/router/utils.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 186351dbb1..755f9a7c2f 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -487,7 +487,8 @@ let _explodeOptionalSegments = (path: string): string[] => { * - `/one/three/:five` (because `/one/three/:four` has priority) * - `/one/:two/three/:five` (because `/one/:two/three/:four` has priority) */ -let explodeOptionalSegments = function* (path: string) { +let explodeOptionalSegments = (path: string) => { + let result: string[] = []; // Compute hash for dynamic path segments // /one/:two/three/:four -> /one/:/three/: let dynamicHash = (subpath: string) => @@ -510,12 +511,13 @@ let explodeOptionalSegments = function* (path: string) { // for absolute paths, ensure `/` instead of empty segment if (path.startsWith("/") && exploded === "") { - yield "/"; + result.push("/"); continue; } - yield exploded; + result.push(exploded); } + return result; }; function rankRouteBranches(branches: RouteBranch[]): void { From 647f412072047ea376d45047d721e79f8d9d19be Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 7 Dec 2022 14:54:10 -0500 Subject: [PATCH 4/6] unit tests --- .../__tests__/path-matching-test.tsx | 321 ++++++++++++++++-- 1 file changed, 298 insertions(+), 23 deletions(-) diff --git a/packages/react-router/__tests__/path-matching-test.tsx b/packages/react-router/__tests__/path-matching-test.tsx index 6d64b180b5..91f430344a 100644 --- a/packages/react-router/__tests__/path-matching-test.tsx +++ b/packages/react-router/__tests__/path-matching-test.tsx @@ -10,20 +10,11 @@ function pickPathsAndParams(routes: RouteObject[], pathname: string) { let matches = matchRoutes(routes, pathname); return ( matches && - matches.map((match) => { - let { path } = match.route - - // filter params to only include params for the current (relative) route path - let paramNames = path?.split('/') - .filter(seg => seg.startsWith(':')) - .map(paramSeg => paramSeg.replace(/^:/, "").replace(/\?$/, "")) - let params = Object.fromEntries(Object.entries(match.params).filter(([k,v]) => paramNames?.includes(k))) - - return { - path: match.route.path, - params, - } - }) + matches.map((match) => ({ + ...(match.route.index ? { index: match.route.index } : {}), + ...(match.route.path ? { path: match.route.path } : {}), + params: match.params, + })) ); } @@ -383,7 +374,6 @@ describe("path matching with optional segments", () => { }, ]; - expect(pickPathsAndParams(nested, "/en/abc")).toEqual([ { path: "/en?", params: {} }, { path: "abc", params: {} }, @@ -481,17 +471,302 @@ describe("path matching with optional dynamic segments", () => { path: "three/:four?", children: [ { - path: ":five?" - } - ] - } + path: ":five?", + }, + ], + }, ], }, ]; + expect(pickPathsAndParams(nested, "/one/dos/three/cuatro/cinco")).toEqual([ + { + path: "/one/:two?", + params: { two: "dos", four: "cuatro", five: "cinco" }, + }, + { + path: "three/:four?", + params: { two: "dos", four: "cuatro", five: "cinco" }, + }, + { path: ":five?", params: { two: "dos", four: "cuatro", five: "cinco" } }, + ]); + expect(pickPathsAndParams(nested, "/one/dos/three/cuatro")).toEqual([ + { + path: "/one/:two?", + params: { two: "dos", four: "cuatro" }, + }, + { + path: "three/:four?", + params: { two: "dos", four: "cuatro" }, + }, + { + path: ":five?", + params: { two: "dos", four: "cuatro" }, + }, + ]); + expect(pickPathsAndParams(nested, "/one/dos/three")).toEqual([ + { + path: "/one/:two?", + params: { two: "dos" }, + }, + { + path: "three/:four?", + params: { two: "dos" }, + }, + // Matches into 5 because it's just like if we did path="" + { + path: ":five?", + params: { two: "dos" }, + }, + ]); + expect(pickPathsAndParams(nested, "/one/dos")).toEqual([ + { + path: "/one/:two?", + params: { two: "dos" }, + }, + ]); + expect(pickPathsAndParams(nested, "/one")).toEqual([ + { + path: "/one/:two?", + params: {}, + }, + ]); expect(pickPathsAndParams(nested, "/one/three/cuatro/cinco")).toEqual([ - {path: "/one/:two?", params: {}}, - {path: "three/:four?", params: { four: 'cuatro' }}, - {path: ":five?", params: { five: 'cinco' }}, - ]) + { + path: "/one/:two?", + params: { four: "cuatro", five: "cinco" }, + }, + { + path: "three/:four?", + params: { four: "cuatro", five: "cinco" }, + }, + { path: ":five?", params: { four: "cuatro", five: "cinco" } }, + ]); + expect(pickPathsAndParams(nested, "/one/three/cuatro")).toEqual([ + { + path: "/one/:two?", + params: { four: "cuatro" }, + }, + { + path: "three/:four?", + params: { four: "cuatro" }, + }, + { + path: ":five?", + params: { four: "cuatro" }, + }, + ]); + expect(pickPathsAndParams(nested, "/one/three")).toEqual([ + { + path: "/one/:two?", + params: {}, + }, + { + path: "three/:four?", + params: {}, + }, + // Matches into 5 because it's just like if we did path="" + { + path: ":five?", + params: {}, + }, + ]); + expect(pickPathsAndParams(nested, "/one")).toEqual([ + { + path: "/one/:two?", + params: {}, + }, + ]); + }); + + test("prefers optional static over optional dynamic segments", () => { + let nested = [ + { + path: "/one", + children: [ + { + path: ":param?", + children: [ + { + path: "three", + }, + ], + }, + { + path: "two?", + children: [ + { + path: "three", + }, + ], + }, + ], + }, + ]; + + // static `two` segment should win + expect(pickPathsAndParams(nested, "/one/two/three")).toEqual([ + { + params: {}, + path: "/one", + }, + { + params: {}, + path: "two?", + }, + { + params: {}, + path: "three", + }, + ]); + + // fall back to param when no static match + expect(pickPathsAndParams(nested, "/one/not-two/three")).toEqual([ + { + params: { + param: "not-two", + }, + path: "/one", + }, + { + params: { + param: "not-two", + }, + path: ":param?", + }, + { + params: { + param: "not-two", + }, + path: "three", + }, + ]); + + // No optional segment provided - earlier "dup" route should win + expect(pickPathsAndParams(nested, "/one/three")).toEqual([ + { + params: {}, + path: "/one", + }, + { + params: {}, + path: ":param?", + }, + { + params: {}, + path: "three", + }, + ]); + }); + + test("prefers index routes over optional static segments", () => { + let nested = [ + { + path: "/one", + children: [ + { + path: ":param?", + children: [ + { + path: "three?", + }, + { + index: true, + }, + ], + }, + ], + }, + ]; + + expect(pickPathsAndParams(nested, "/one/two")).toEqual([ + { + params: { + param: "two", + }, + path: "/one", + }, + { + params: { + param: "two", + }, + path: ":param?", + }, + { + index: true, + params: { + param: "two", + }, + }, + ]); + expect(pickPathsAndParams(nested, "/one")).toEqual([ + { + params: {}, + path: "/one", + }, + { + params: {}, + path: ":param?", + }, + { + index: true, + params: {}, + }, + ]); + }); + + test("prefers index routes over optional dynamic segments", () => { + let nested = [ + { + path: "/one", + children: [ + { + path: ":param?", + children: [ + { + path: ":three?", + }, + { + index: true, + }, + ], + }, + ], + }, + ]; + + expect(pickPathsAndParams(nested, "/one/two")).toEqual([ + { + params: { + param: "two", + }, + path: "/one", + }, + { + params: { + param: "two", + }, + path: ":param?", + }, + { + index: true, + params: { + param: "two", + }, + }, + ]); + expect(pickPathsAndParams(nested, "/one")).toEqual([ + { + params: {}, + path: "/one", + }, + { + params: {}, + path: ":param?", + }, + { + index: true, + params: {}, + }, + ]); }); }); From fa7f93394fa70afa84e97ae145f2fcd997dbfa87 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 7 Dec 2022 15:08:46 -0500 Subject: [PATCH 5/6] Remove dynamic hash and collapse into _explodeOptionalSegments --- packages/router/utils.ts | 92 +++++++++++++--------------------------- 1 file changed, 30 insertions(+), 62 deletions(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 755f9a7c2f..a214d8bc36 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -440,9 +440,20 @@ function flattenRoutes< } /** - * Computes all combinations of optional path segments for a given path. + * Computes all combinations of optional path segments for a given path, + * excluding combinations that are ambiguous and of lower priority. + * + * For example, `/one/:two?/three/:four?/:five?` explodes to: + * - `/one/three` + * - `/one/:two/three` + * - `/one/three/:four` + * - `/one/three/:five` + * - `/one/:two/three/:four` + * - `/one/:two/three/:five` + * - `/one/three/:four/:five` + * - `/one/:two/three/:four/:five` */ -let _explodeOptionalSegments = (path: string): string[] => { +function explodeOptionalSegments(path: string): string[] { let segments = path.split("/"); if (segments.length === 0) return []; @@ -459,66 +470,23 @@ let _explodeOptionalSegments = (path: string): string[] => { return isOptional ? ["", required] : [required]; } - let restExploded = _explodeOptionalSegments(rest.join("/")); - return restExploded.flatMap((subpath) => { - // /one + / + :two/three -> /one/:two/three - let requiredExploded = subpath === "" ? required : required + "/" + subpath; - // For optional segments, return the exploded path _without_ current segment first (`subpath`) - // and exploded path _with_ current segment later (`subpath`) - // This ensures that exploded paths are emitted in priority order - // `/one/three/:four` will come before `/one/three/:five` - return isOptional ? [subpath, requiredExploded] : [requiredExploded]; - }); -}; - -/** - * Computes all combinations of optional path segments for a given path, - * excluding combinations that are ambiguous and of lower priority. - * - * For example, `/one/:two?/three/:four?/:five?` explodes to: - * - `/one/three` - * - `/one/:two/three` - * - `/one/three/:four` - * - `/one/:two/three/:four` - * - `/one/three/:four/:five` - * - `/one/:two/three/:four/:five` - * - * Note that these paths are not returned: - * - `/one/three/:five` (because `/one/three/:four` has priority) - * - `/one/:two/three/:five` (because `/one/:two/three/:four` has priority) - */ -let explodeOptionalSegments = (path: string) => { - let result: string[] = []; - // Compute hash for dynamic path segments - // /one/:two/three/:four -> /one/:/three/: - let dynamicHash = (subpath: string) => - subpath - .split("/") - .map((segment) => (segment.startsWith(":") ? ":" : segment)) - .join("/"); - - let dynamicHashes = new Set(); - for (let exploded of _explodeOptionalSegments(path)) { - let hash = dynamicHash(exploded); - - // `/one/three/:four` and `/one/three/:five` have the same hash: `/one/three/:` - // so we only emit the first one of these we come across - // _explodeOptionalSegments returns exploded paths in priority order, - // so `/one/three/:four` will come before `/one/three/:five` - if (dynamicHashes.has(hash)) continue; - - dynamicHashes.add(hash); - - // for absolute paths, ensure `/` instead of empty segment - if (path.startsWith("/") && exploded === "") { - result.push("/"); - continue; - } - - result.push(exploded); - } - return result; -}; + let restExploded = explodeOptionalSegments(rest.join("/")); + return restExploded + .flatMap((subpath) => { + // /one + / + :two/three -> /one/:two/three + let requiredExploded = + subpath === "" ? required : required + "/" + subpath; + // For optional segments, return the exploded path _without_ current segment first (`subpath`) + // and exploded path _with_ current segment later (`subpath`) + // This ensures that exploded paths are emitted in priority order + // `/one/three/:four` will come before `/one/three/:five` + return isOptional ? [subpath, requiredExploded] : [requiredExploded]; + }) + .map((exploded) => { + // for absolute paths, ensure `/` instead of empty segment + return path.startsWith("/") && exploded === "" ? "/" : exploded; + }); +} function rankRouteBranches(branches: RouteBranch[]): void { branches.sort((a, b) => From 6f4c8241f6919c50f340fda15478f26b4902a074 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Wed, 7 Dec 2022 15:31:45 -0500 Subject: [PATCH 6/6] refactor: remove nullish coalescing `??` to be browser compatible --- packages/router/utils.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index a214d8bc36..6ed5604f1a 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -378,7 +378,8 @@ function flattenRoutes< relativePath?: string ) => { let meta: RouteMeta = { - relativePath: relativePath ?? (route.path || ""), + relativePath: + relativePath === undefined ? route.path || "" : relativePath, caseSensitive: route.caseSensitive === true, childrenIndex: index, route,