Merge pull request #9727 from remix-run/brophdawg11/fix-optional-segm…

…ents-bug

fix bug with nested optional segments

.changeset/slow-drinks-cheer.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+Fix issue with deeply nested optional segments

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?",

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 {