From c282a5dd6ca41450136fb90605d335bfb3363812 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Sep 2022 11:14:56 -0400 Subject: [PATCH 1/8] fix: ensure consistency in generatePath/compilePath for partial splat params --- .../__tests__/generatePath-test.tsx | 27 +++++++++- .../__tests__/path-matching-test.tsx | 53 +++++++++++++++++++ packages/router/utils.ts | 14 ++++- 3 files changed, 91 insertions(+), 3 deletions(-) diff --git a/packages/react-router/__tests__/generatePath-test.tsx b/packages/react-router/__tests__/generatePath-test.tsx index 9276055aa5..4f095b9726 100644 --- a/packages/react-router/__tests__/generatePath-test.tsx +++ b/packages/react-router/__tests__/generatePath-test.tsx @@ -51,6 +51,8 @@ describe("generatePath", () => { }); it("only interpolates and does not add slashes", () => { + let consoleWarn = jest.spyOn(console, "warn").mockImplementation(() => {}); + expect(generatePath("*")).toBe(""); expect(generatePath("/*")).toBe("/"); @@ -66,7 +68,28 @@ describe("generatePath", () => { expect(generatePath("foo:bar", { bar: "baz" })).toBe("foobaz"); expect(generatePath("/foo:bar", { bar: "baz" })).toBe("/foobaz"); - expect(generatePath("foo*", { "*": "bar" })).toBe("foobar"); - expect(generatePath("/foo*", { "*": "bar" })).toBe("/foobar"); + // 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/*\\".", + ], + ] + `); + + consoleWarn.mockRestore(); }); }); diff --git a/packages/react-router/__tests__/path-matching-test.tsx b/packages/react-router/__tests__/path-matching-test.tsx index dd5e58a3af..48bfeb2cbc 100644 --- a/packages/react-router/__tests__/path-matching-test.tsx +++ b/packages/react-router/__tests__/path-matching-test.tsx @@ -267,4 +267,57 @@ describe("path matching with splats", () => { pathnameBase: "/courses", }); }); + + test("supports partial path matching with named parameters", () => { + let routes = [{ path: "/prefix:id" }]; + expect(matchRoutes(routes, "/prefixabc")).toMatchInlineSnapshot(` + Array [ + Object { + "params": Object { + "id": "abc", + }, + "pathname": "/prefixabc", + "pathnameBase": "/prefixabc", + "route": Object { + "path": "/prefix:id", + }, + }, + ] + `); + expect(matchRoutes(routes, "/prefix/abc")).toMatchInlineSnapshot(`null`); + }); + + test("does not support partial path matching with named 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(); + }); }); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 70761f6146..502d0aba6f 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -528,11 +528,23 @@ function matchRouteBranch< * @see https://reactrouter.com/docs/en/v6/utils/generate-path */ export function generatePath( - path: Path, + originalPath: Path, params: { [key in PathParam]: 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) => { invariant(params[key] != null, `Missing ":${key}" param`); From 4f3d632f84f775f3179875da56bb32d4dbae3ac2 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 26 Oct 2022 15:14:09 -0400 Subject: [PATCH 2/8] Do not match partial dynamic parameters --- .../__tests__/generatePath-test.tsx | 11 +++++++++-- .../__tests__/path-matching-test.tsx | 17 ++++++++--------- .../react-router/__tests__/useRoutes-test.tsx | 13 ++++++++++++- packages/router/utils.ts | 10 +++++++--- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/packages/react-router/__tests__/generatePath-test.tsx b/packages/react-router/__tests__/generatePath-test.tsx index 4f095b9726..105a2ed63f 100644 --- a/packages/react-router/__tests__/generatePath-test.tsx +++ b/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" ); @@ -65,8 +71,9 @@ 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"); diff --git a/packages/react-router/__tests__/path-matching-test.tsx b/packages/react-router/__tests__/path-matching-test.tsx index 48bfeb2cbc..74188424cd 100644 --- a/packages/react-router/__tests__/path-matching-test.tsx +++ b/packages/react-router/__tests__/path-matching-test.tsx @@ -268,26 +268,25 @@ describe("path matching with splats", () => { }); }); - test("supports partial path matching with named parameters", () => { + test("does not support partial path matching with named parameters", () => { let routes = [{ path: "/prefix:id" }]; - expect(matchRoutes(routes, "/prefixabc")).toMatchInlineSnapshot(` + expect(matchRoutes(routes, "/prefix:id")).toMatchInlineSnapshot(` Array [ Object { - "params": Object { - "id": "abc", - }, - "pathname": "/prefixabc", - "pathnameBase": "/prefixabc", + "params": Object {}, + "pathname": "/prefix:id", + "pathnameBase": "/prefix:id", "route": Object { "path": "/prefix:id", }, }, ] `); - expect(matchRoutes(routes, "/prefix/abc")).toMatchInlineSnapshot(`null`); + expect(matchRoutes(routes, "/prefixabc")).toEqual(null); + expect(matchRoutes(routes, "/prefix/abc")).toEqual(null); }); - test("does not support partial path matching with named parameters", () => { + 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(` diff --git a/packages/react-router/__tests__/useRoutes-test.tsx b/packages/react-router/__tests__/useRoutes-test.tsx index 18e200f1fa..91de38a632 100644 --- a/packages/react-router/__tests__/useRoutes-test.tsx +++ b/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:

one

}]; const NullRenderer = (props: { routes: RouteObject[] }) => { @@ -97,9 +99,13 @@ describe("useRoutes", () => { is null `); + + 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:

one

}]; const NullRenderer = (props: { @@ -114,7 +120,10 @@ describe("useRoutes", () => { TestRenderer.act(() => { renderer = TestRenderer.create( - + ); }); @@ -124,6 +133,8 @@ describe("useRoutes", () => { is null `); + + spy.mockRestore(); }); describe("warns", () => { diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 502d0aba6f..6470de15c2 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -546,10 +546,14 @@ export function generatePath( } return path - .replace(/:(\w+)/g, (_, key: PathParam) => { + .replace(/^:(\w+)/g, (_, key: PathParam) => { invariant(params[key] != null, `Missing ":${key}" param`); return params[key]!; }) + .replace(/\/:(\w+)/g, (_, key: PathParam) => { + invariant(params[key] != null, `Missing ":${key}" param`); + return `/${params[key]!}`; + }) .replace(/(\/?)\*/, (_, prefix, __, str) => { const star = "*" as PathParam; @@ -688,9 +692,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("*")) { From a5c5059f08e0298ee476d336efd79894dd0cd1ec Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 26 Oct 2022 15:54:21 -0400 Subject: [PATCH 3/8] bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index b8f4abe777..02d95982ed 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ }, "filesize": { "packages/router/dist/router.js": { - "none": "108 kB" + "none": "109 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "12.5 kB" From 4bbeb90900decc9869159b202451ccc0a4965337 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 26 Oct 2022 16:02:43 -0400 Subject: [PATCH 4/8] add changeset --- .changeset/happy-balloons-buy.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 .changeset/happy-balloons-buy.md diff --git a/.changeset/happy-balloons-buy.md b/.changeset/happy-balloons-buy.md new file mode 100644 index 0000000000..97655a2aae --- /dev/null +++ b/.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. ``, 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 + }> + +function Comp() { + let params = useParams(); // { id: '123' } + let id = params.id; // "123" + ... +} + +// New behavior at URL /prefix-123 + }> + +function Comp() { + let params = useParams(); // { id: 'prefix-123' } + let id = params.id.replace(/^prefix-/, ''); // "123" + ... +} +``` From c2c250c2e8a04e391e2db9ff71972e802bc737f7 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 15 Nov 2022 15:34:28 -0500 Subject: [PATCH 5/8] fix changeset backtick --- .changeset/happy-balloons-buy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/happy-balloons-buy.md b/.changeset/happy-balloons-buy.md index 97655a2aae..1c612530da 100644 --- a/.changeset/happy-balloons-buy.md +++ b/.changeset/happy-balloons-buy.md @@ -3,7 +3,7 @@ "@remix-run/router": patch --- -Stop incorrectly matching on partial named parameters, i.e. ``, 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: +Stop incorrectly matching on partial named parameters, i.e. ``, 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 From 114e26f20284a4296742b9c6f8167c0a17632f7a Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 15 Nov 2022 15:37:28 -0500 Subject: [PATCH 6/8] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 46a937dd23..bbce7aa04e 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "34.5 kB" + "none": "35 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "12.5 kB" From 5083d6848feb24f396d9bda7f199e9c182c8555f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 18 Nov 2022 09:58:20 -0500 Subject: [PATCH 7/8] Update type --- packages/router/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 4f35f3b889..ee889e1c75 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -194,7 +194,7 @@ type _PathParam = Path extends `${infer L}/${infer R}` ? _PathParam | _PathParam : // find params after `:` - Path extends `${string}:${infer Param}` + Path extends `:${infer Param}` ? Param : // otherwise, there aren't any params present never; From db7a8e62de9b33382337869bcddf14806d60eea5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 30 Nov 2022 16:02:50 -0500 Subject: [PATCH 8/8] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index d7b6afc2db..9838ae7e99 100644 --- a/package.json +++ b/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"