Fix inadvertent support for partial dynamic parameters (#9506)

* fix: ensure consistency in generatePath/compilePath for partial splat params
* Do not match partial dynamic parameters

Author: brophdawg11

.changeset/happy-balloons-buy.md

@@ -0,0 +1,26 @@
+---
+"react-router": patch
+"@remix-run/router": patch
+---
+
+Stop incorrectly matching on partial named parameters, i.e. `<Route path="prefix-:param">`, to align with how splat parameters work. If you were previously relying on this behavior then it's recommended to extract the static portion of the path at the `useParams` call site:
+
+```jsx
+// Old behavior at URL /prefix-123
+<Route path="prefix-:id" element={<Comp /> }>
+
+function Comp() {
+  let params = useParams(); // { id: '123' }
+  let id = params.id; // "123"
+  ...
+}
+
+// New behavior at URL /prefix-123
+<Route path=":id" element={<Comp /> }>
+
+function Comp() {
+  let params = useParams(); // { id: 'prefix-123' }
+  let id = params.id.replace(/^prefix-/, ''); // "123"
+  ...
+}
+```

package.json

@@ -107,7 +107,7 @@
   },
   "filesize": {
     "packages/router/dist/router.umd.min.js": {
-      "none": "35.5 kB"
+      "none": "36 kB"
     },
     "packages/react-router/dist/react-router.production.min.js": {
       "none": "12.5 kB"

packages/react-router/__tests__/generatePath-test.tsx

@@ -13,6 +13,12 @@ describe("generatePath", () => {
       expect(generatePath("/courses/:id", { id: "routing" })).toBe(
         "/courses/routing"
       );
+      expect(
+        generatePath("/courses/:id/student/:studentId", {
+          id: "routing",
+          studentId: "matt",
+        })
+      ).toBe("/courses/routing/student/matt");
       expect(generatePath("/courses/*", { "*": "routing/grades" })).toBe(
         "/courses/routing/grades"
       );
@@ -51,6 +57,8 @@ describe("generatePath", () => {
   });
 
   it("only interpolates and does not add slashes", () => {
+    let consoleWarn = jest.spyOn(console, "warn").mockImplementation(() => {});
+
     expect(generatePath("*")).toBe("");
     expect(generatePath("/*")).toBe("/");
 
@@ -63,10 +71,32 @@ describe("generatePath", () => {
     expect(generatePath("*", { "*": "bar" })).toBe("bar");
     expect(generatePath("/*", { "*": "bar" })).toBe("/bar");
 
-    expect(generatePath("foo:bar", { bar: "baz" })).toBe("foobaz");
-    expect(generatePath("/foo:bar", { bar: "baz" })).toBe("/foobaz");
+    // No support for partial dynamic params
+    expect(generatePath("foo:bar", { bar: "baz" })).toBe("foo:bar");
+    expect(generatePath("/foo:bar", { bar: "baz" })).toBe("/foo:bar");
+
+    // Partial splats are treated as independent path segments
+    expect(generatePath("foo*", { "*": "bar" })).toBe("foo/bar");
+    expect(generatePath("/foo*", { "*": "bar" })).toBe("/foo/bar");
+
+    // Ensure we warn on partial splat usages
+    expect(consoleWarn.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          "Route path \\"foo*\\" will be treated as if it were \\"foo/*\\" because the \`*\` character must always follow a \`/\` in the pattern. To get rid of this warning, please change the route path to \\"foo/*\\".",
+        ],
+        Array [
+          "Route path \\"/foo*\\" will be treated as if it were \\"/foo/*\\" because the \`*\` character must always follow a \`/\` in the pattern. To get rid of this warning, please change the route path to \\"/foo/*\\".",
+        ],
+        Array [
+          "Route path \\"foo*\\" will be treated as if it were \\"foo/*\\" because the \`*\` character must always follow a \`/\` in the pattern. To get rid of this warning, please change the route path to \\"foo/*\\".",
+        ],
+        Array [
+          "Route path \\"/foo*\\" will be treated as if it were \\"/foo/*\\" because the \`*\` character must always follow a \`/\` in the pattern. To get rid of this warning, please change the route path to \\"/foo/*\\".",
+        ],
+      ]
+    `);
 
-    expect(generatePath("foo*", { "*": "bar" })).toBe("foobar");
-    expect(generatePath("/foo*", { "*": "bar" })).toBe("/foobar");
+    consoleWarn.mockRestore();
   });
 });

packages/react-router/__tests__/path-matching-test.tsx

@@ -275,6 +275,58 @@ describe("path matching with splats", () => {
       pathnameBase: "/courses",
     });
   });
+
+  test("does not support partial path matching with named parameters", () => {
+    let routes = [{ path: "/prefix:id" }];
+    expect(matchRoutes(routes, "/prefix:id")).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "params": Object {},
+          "pathname": "/prefix:id",
+          "pathnameBase": "/prefix:id",
+          "route": Object {
+            "path": "/prefix:id",
+          },
+        },
+      ]
+    `);
+    expect(matchRoutes(routes, "/prefixabc")).toEqual(null);
+    expect(matchRoutes(routes, "/prefix/abc")).toEqual(null);
+  });
+
+  test("does not support partial path matching with splat parameters", () => {
+    let consoleWarn = jest.spyOn(console, "warn").mockImplementation(() => {});
+    let routes = [{ path: "/prefix*" }];
+    expect(matchRoutes(routes, "/prefix/abc")).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "params": Object {
+            "*": "abc",
+          },
+          "pathname": "/prefix/abc",
+          "pathnameBase": "/prefix",
+          "route": Object {
+            "path": "/prefix*",
+          },
+        },
+      ]
+    `);
+    expect(matchRoutes(routes, "/prefixabc")).toMatchInlineSnapshot(`null`);
+
+    // Should warn on each invocation of matchRoutes
+    expect(consoleWarn.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          "Route path \\"/prefix*\\" will be treated as if it were \\"/prefix/*\\" because the \`*\` character must always follow a \`/\` in the pattern. To get rid of this warning, please change the route path to \\"/prefix/*\\".",
+        ],
+        Array [
+          "Route path \\"/prefix*\\" will be treated as if it were \\"/prefix/*\\" because the \`*\` character must always follow a \`/\` in the pattern. To get rid of this warning, please change the route path to \\"/prefix/*\\".",
+        ],
+      ]
+    `);
+
+    consoleWarn.mockRestore();
+  });
 });
 
 describe("path matchine with optional segments", () => {

packages/react-router/__tests__/useRoutes-test.tsx

@@ -76,6 +76,8 @@ describe("useRoutes", () => {
   });
 
   it("returns null when no route matches", () => {
+    let spy = jest.spyOn(console, "warn").mockImplementation(() => {});
+
     let routes = [{ path: "one", element: <h1>one</h1> }];
 
     const NullRenderer = (props: { routes: RouteObject[] }) => {
@@ -97,9 +99,13 @@ describe("useRoutes", () => {
         is null
       </div>
     `);
+
+    spy.mockRestore();
   });
 
   it("returns null when no route matches and a `location` prop is passed", () => {
+    let spy = jest.spyOn(console, "warn").mockImplementation(() => {});
+
     let routes = [{ path: "one", element: <h1>one</h1> }];
 
     const NullRenderer = (props: {
@@ -114,7 +120,10 @@ describe("useRoutes", () => {
     TestRenderer.act(() => {
       renderer = TestRenderer.create(
         <MemoryRouter initialEntries={["/two"]}>
-          <NullRenderer routes={routes} location={{ pathname: "/three" }} />
+          <NullRenderer
+            routes={routes}
+            location={{ pathname: "/three", search: "", hash: "" }}
+          />
         </MemoryRouter>
       );
     });
@@ -124,6 +133,8 @@ describe("useRoutes", () => {
         is null
       </div>
     `);
+
+    spy.mockRestore();
   });
 
   describe("warns", () => {

packages/router/utils.ts

@@ -197,7 +197,7 @@ type _PathParam<Path extends string> =
   Path extends `${infer L}/${infer R}`
     ? _PathParam<L> | _PathParam<R>
     : // find params after `:`
-    Path extends `${string}:${infer Param}`
+    Path extends `:${infer Param}`
     ? Param
     : // otherwise, there aren't any params present
       never;
@@ -570,16 +570,32 @@ function matchRouteBranch<
  * @see https://reactrouter.com/docs/en/v6/utils/generate-path
  */
 export function generatePath<Path extends string>(
-  path: Path,
+  originalPath: Path,
   params: {
     [key in PathParam<Path>]: string;
   } = {} as any
 ): string {
+  let path = originalPath;
+  if (path.endsWith("*") && path !== "*" && !path.endsWith("/*")) {
+    warning(
+      false,
+      `Route path "${path}" will be treated as if it were ` +
+        `"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` +
+        `always follow a \`/\` in the pattern. To get rid of this warning, ` +
+        `please change the route path to "${path.replace(/\*$/, "/*")}".`
+    );
+    path = path.replace(/\*$/, "/*") as Path;
+  }
+
   return path
-    .replace(/:(\w+)/g, (_, key: PathParam<Path>) => {
+    .replace(/^:(\w+)/g, (_, key: PathParam<Path>) => {
       invariant(params[key] != null, `Missing ":${key}" param`);
       return params[key]!;
     })
+    .replace(/\/:(\w+)/g, (_, key: PathParam<Path>) => {
+      invariant(params[key] != null, `Missing ":${key}" param`);
+      return `/${params[key]!}`;
+    })
     .replace(/(\/?)\*/, (_, prefix, __, str) => {
       const star = "*" as PathParam<Path>;
 
@@ -718,9 +734,9 @@ function compilePath(
       .replace(/\/*\*?$/, "") // Ignore trailing / and /*, we'll handle it below
       .replace(/^\/*/, "/") // Make sure it has a leading /
       .replace(/[\\.*+^$?{}|()[\]]/g, "\\$&") // Escape special regex chars
-      .replace(/:(\w+)/g, (_: string, paramName: string) => {
+      .replace(/\/:(\w+)/g, (_: string, paramName: string) => {
         paramNames.push(paramName);
-        return "([^\\/]+)";
+        return "/([^\\/]+)";
       });
 
   if (path.endsWith("*")) {