From e94438b646ddebb690b2dda1d01a7e33e9d643d0 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 13 Dec 2022 17:42:09 -0500 Subject: [PATCH 1/2] fix bug with nested optional segments --- .../__tests__/path-matching-test.tsx | 240 +++++++++++++++++- packages/router/utils.ts | 43 ++-- 2 files changed, 262 insertions(+), 21 deletions(-) diff --git a/packages/react-router/__tests__/path-matching-test.tsx b/packages/react-router/__tests__/path-matching-test.tsx index 40ccd8c6cc..fed9dc703c 100644 --- a/packages/react-router/__tests__/path-matching-test.tsx +++ b/packages/react-router/__tests__/path-matching-test.tsx @@ -458,31 +458,97 @@ describe("path matching with optional dynamic segments", () => { }); test("optional params at the end of the path", () => { + let manualRoutes = [ + { + path: "/nested", + }, + { + path: "/nested/:one", + }, + { + path: "/nested/:one/:two", + }, + { + path: "/nested/:one/:two/:three", + }, + { + path: "/nested/:one/:two/:three/:four", + }, + ]; let routes = [ { - path: "/nested/:one?/:two?", + path: "/nested/:one?/:two?/:three?/:four?", }, ]; + expect(pickPathsAndParams(manualRoutes, "/nested")).toEqual([ + { + path: "/nested", + params: {}, + }, + ]); expect(pickPathsAndParams(routes, "/nested")).toEqual([ { - path: "/nested/:one?/:two?", + path: "/nested/:one?/:two?/:three?/:four?", params: {}, }, ]); + expect(pickPathsAndParams(manualRoutes, "/nested/foo")).toEqual([ + { + path: "/nested/:one", + params: { one: "foo" }, + }, + ]); expect(pickPathsAndParams(routes, "/nested/foo")).toEqual([ { - path: "/nested/:one?/:two?", + path: "/nested/:one?/:two?/:three?/:four?", params: { one: "foo" }, }, ]); + expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar")).toEqual([ + { + path: "/nested/:one/:two", + params: { one: "foo", two: "bar" }, + }, + ]); expect(pickPathsAndParams(routes, "/nested/foo/bar")).toEqual([ { - path: "/nested/:one?/:two?", + path: "/nested/:one?/:two?/:three?/:four?", params: { one: "foo", two: "bar" }, }, ]); - expect(pickPathsAndParams(routes, "/nested/foo/bar/baz")).toEqual(null); + expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz")).toEqual([ + { + path: "/nested/:one/:two/:three", + params: { one: "foo", two: "bar", three: "baz" }, + }, + ]); + expect(pickPathsAndParams(routes, "/nested/foo/bar/baz")).toEqual([ + { + path: "/nested/:one?/:two?/:three?/:four?", + params: { one: "foo", two: "bar", three: "baz" }, + }, + ]); + expect(pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz/qux")).toEqual( + [ + { + path: "/nested/:one/:two/:three/:four", + params: { one: "foo", two: "bar", three: "baz", four: "qux" }, + }, + ] + ); + expect(pickPathsAndParams(routes, "/nested/foo/bar/baz/qux")).toEqual([ + { + path: "/nested/:one?/:two?/:three?/:four?", + params: { one: "foo", two: "bar", three: "baz", four: "qux" }, + }, + ]); + expect( + pickPathsAndParams(manualRoutes, "/nested/foo/bar/baz/qux/zod") + ).toEqual(null); + expect(pickPathsAndParams(routes, "/nested/foo/bar/baz/qux/zod")).toEqual( + null + ); }); test("intercalated optional params", () => { @@ -515,6 +581,170 @@ describe("path matching with optional dynamic segments", () => { }); test("consecutive optional dynamic segments in nested routes", () => { + let manuallyExploded = [ + { + path: ":one", + children: [ + { + path: ":two", + children: [ + { + path: ":three", + }, + { + path: "", + }, + ], + }, + { + path: "", + children: [ + { + path: ":three", + }, + { + path: "", + }, + ], + }, + ], + }, + { + path: "", + children: [ + { + path: ":two", + children: [ + { + path: ":three", + }, + { + path: "", + }, + ], + }, + { + path: "", + children: [ + { + path: ":three", + }, + { + path: "", + }, + ], + }, + ], + }, + ]; + + let optional = [ + { + path: ":one?", + children: [ + { + path: ":two?", + children: [ + { + path: ":three?", + }, + ], + }, + ], + }, + ]; + + expect(pickPathsAndParams(manuallyExploded, "/uno")).toEqual([ + { + path: ":one", + params: { one: "uno" }, + }, + { + params: { one: "uno" }, + }, + { + params: { one: "uno" }, + }, + ]); + expect(pickPathsAndParams(optional, "/uno")).toEqual([ + { + path: ":one?", + params: { one: "uno" }, + }, + { + params: { one: "uno" }, + path: ":two?", + }, + { + params: { one: "uno" }, + path: ":three?", + }, + ]); + + expect(pickPathsAndParams(manuallyExploded, "/uno/dos")).toEqual([ + { + path: ":one", + params: { one: "uno", two: "dos" }, + }, + { + params: { one: "uno", two: "dos" }, + path: ":two", + }, + { + params: { one: "uno", two: "dos" }, + }, + ]); + expect(pickPathsAndParams(optional, "/uno/dos")).toEqual([ + { + path: ":one?", + params: { one: "uno", two: "dos" }, + }, + { + params: { one: "uno", two: "dos" }, + path: ":two?", + }, + { + params: { one: "uno", two: "dos" }, + path: ":three?", + }, + ]); + + expect(pickPathsAndParams(manuallyExploded, "/uno/dos/tres")).toEqual([ + { + path: ":one", + params: { one: "uno", two: "dos", three: "tres" }, + }, + { + params: { one: "uno", two: "dos", three: "tres" }, + path: ":two", + }, + { + params: { one: "uno", two: "dos", three: "tres" }, + path: ":three", + }, + ]); + expect(pickPathsAndParams(optional, "/uno/dos/tres")).toEqual([ + { + path: ":one?", + params: { one: "uno", two: "dos", three: "tres" }, + }, + { + params: { one: "uno", two: "dos", three: "tres" }, + path: ":two?", + }, + { + params: { one: "uno", two: "dos", three: "tres" }, + path: ":three?", + }, + ]); + + expect(pickPathsAndParams(manuallyExploded, "/uno/dos/tres/nope")).toEqual( + null + ); + expect(pickPathsAndParams(optional, "/uno/dos/tres/nope")).toEqual(null); + }); + + test("consecutive optional static + dynamic segments in nested routes", () => { let nested = [ { path: "/one/:two?", diff --git a/packages/router/utils.ts b/packages/router/utils.ts index faa778dfb0..f9aa40a411 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -329,6 +329,7 @@ export function matchRoutes< let branches = flattenRoutes(routes); rankRouteBranches(branches); + console.log(branches.map((b) => ({ path: b.path, score: b.score }))); let matches = null; for (let i = 0; matches == null && i < branches.length; ++i) { @@ -468,25 +469,35 @@ function explodeOptionalSegments(path: string): string[] { 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]; + 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]; - }) - .map((exploded) => { - // for absolute paths, ensure `/` instead of empty segment - return path.startsWith("/") && exploded === "" ? "/" : exploded; - }); + + let result: string[] = []; + + // All child paths with the prefix. Do this for all children before the + // optional version for all children so we get consistent ordering where the + // parent optional aspect is preferred as required. Otherwise, we can get + // child sections interspersed where deeper optional segments are higher than + // parent optional segments, where for example, /:two would explodes _earlier_ + // then /:one. By always including the parent as required _for all children_ + // first, we avoid this issue + result.push( + ...restExploded.map((subpath) => + subpath === "" ? required : [required, subpath].join("/") + ) + ); + + // Then if this is an optional value, add all child versions without + if (isOptional) { + result.push(...restExploded); + } + + // for absolute paths, ensure `/` instead of empty segment + return result.map((exploded) => + path.startsWith("/") && exploded === "" ? "/" : exploded + ); } function rankRouteBranches(branches: RouteBranch[]): void { From 0ce082eef31ccb588fa56b7514701f4b527596ec Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 13 Dec 2022 17:52:13 -0500 Subject: [PATCH 2/2] Add changeset and remove console.log --- .changeset/slow-drinks-cheer.md | 5 +++++ packages/router/utils.ts | 1 - 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 .changeset/slow-drinks-cheer.md diff --git a/.changeset/slow-drinks-cheer.md b/.changeset/slow-drinks-cheer.md new file mode 100644 index 0000000000..828e8119ac --- /dev/null +++ b/.changeset/slow-drinks-cheer.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Fix issue with deeply nested optional segments diff --git a/packages/router/utils.ts b/packages/router/utils.ts index f9aa40a411..84c8b2e900 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -329,7 +329,6 @@ export function matchRoutes< let branches = flattenRoutes(routes); rankRouteBranches(branches); - console.log(branches.map((b) => ({ path: b.path, score: b.score }))); let matches = null; for (let i = 0; matches == null && i < branches.length; ++i) {