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/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..84c8b2e900 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -468,25 +468,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 {