From ecfba2b13302b163bc8919e3670482f0de6c28b2 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Sep 2022 11:14:56 -0400 Subject: [PATCH 1/7] 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 eb73f90673..3bab89bc2d 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -475,11 +475,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 d9f64de5d63128dea208d4543b1ef667661a1554 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Sep 2022 11:19:21 -0400 Subject: [PATCH 2/7] Add changeset --- .changeset/angry-pens-flow.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/angry-pens-flow.md diff --git a/.changeset/angry-pens-flow.md b/.changeset/angry-pens-flow.md new file mode 100644 index 0000000000..7db066f555 --- /dev/null +++ b/.changeset/angry-pens-flow.md @@ -0,0 +1,6 @@ +--- +"react-router": patch +"@remix-run/router": patch +--- + +fix: Align behavior of generatePath with compilePath to not support partial splat params (#9238) From 7ed69ab10e604bcdefe7b67187e16797b60eeea7 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Sep 2022 17:34:07 -0400 Subject: [PATCH 3/7] feat: support partial splat path matching --- .../__tests__/generatePath-test.tsx | 27 +---- .../react-router/__tests__/matchPath-test.tsx | 46 +++----- .../__tests__/path-matching-test.tsx | 104 +++++++++++++++--- packages/router/utils.ts | 85 +++++++------- 4 files changed, 148 insertions(+), 114 deletions(-) diff --git a/packages/react-router/__tests__/generatePath-test.tsx b/packages/react-router/__tests__/generatePath-test.tsx index 4f095b9726..9276055aa5 100644 --- a/packages/react-router/__tests__/generatePath-test.tsx +++ b/packages/react-router/__tests__/generatePath-test.tsx @@ -51,8 +51,6 @@ describe("generatePath", () => { }); it("only interpolates and does not add slashes", () => { - let consoleWarn = jest.spyOn(console, "warn").mockImplementation(() => {}); - expect(generatePath("*")).toBe(""); expect(generatePath("/*")).toBe("/"); @@ -68,28 +66,7 @@ describe("generatePath", () => { expect(generatePath("foo:bar", { bar: "baz" })).toBe("foobaz"); expect(generatePath("/foo:bar", { bar: "baz" })).toBe("/foobaz"); - // 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(); + expect(generatePath("foo*", { "*": "bar" })).toBe("foobar"); + expect(generatePath("/foo*", { "*": "bar" })).toBe("/foobar"); }); }); diff --git a/packages/react-router/__tests__/matchPath-test.tsx b/packages/react-router/__tests__/matchPath-test.tsx index 795e889ce4..e2917ea9af 100644 --- a/packages/react-router/__tests__/matchPath-test.tsx +++ b/packages/react-router/__tests__/matchPath-test.tsx @@ -298,38 +298,24 @@ describe("matchPath *", () => { }); }); -describe("matchPath warnings", () => { - let consoleWarn: jest.SpyInstance; - beforeEach(() => { - consoleWarn = jest.spyOn(console, "warn").mockImplementation(); - }); - - afterEach(() => { - consoleWarn.mockRestore(); - }); - - describe("when the pattern has a trailing *", () => { - it("issues a warning and matches the remaining portion of the pathname", () => { - expect(matchPath("/files*", "/files/mj.jpg")).toMatchObject({ - params: { "*": "mj.jpg" }, - pathname: "/files/mj.jpg", - pathnameBase: "/files", - }); - expect(consoleWarn).toHaveBeenCalledTimes(1); +describe("when the pattern has a trailing *", () => { + it("matches the remaining portion of the pathname", () => { + expect(matchPath("/files*", "/files/mj.jpg")).toMatchObject({ + params: { "*": "/mj.jpg" }, + pathname: "/files/mj.jpg", + pathnameBase: "/files", + }); - expect(matchPath("/files*", "/files/")).toMatchObject({ - params: { "*": "" }, - pathname: "/files/", - pathnameBase: "/files", - }); - expect(consoleWarn).toHaveBeenCalledTimes(2); + expect(matchPath("/files*", "/files/")).toMatchObject({ + params: { "*": "/" }, + pathname: "/files/", + pathnameBase: "/files", + }); - expect(matchPath("/files*", "/files")).toMatchObject({ - params: { "*": "" }, - pathname: "/files", - pathnameBase: "/files", - }); - expect(consoleWarn).toHaveBeenCalledTimes(3); + expect(matchPath("/files*", "/files")).toMatchObject({ + params: { "*": "" }, + pathname: "/files", + pathnameBase: "/files", }); }); }); diff --git a/packages/react-router/__tests__/path-matching-test.tsx b/packages/react-router/__tests__/path-matching-test.tsx index 48bfeb2cbc..5e1d8cceb9 100644 --- a/packages/react-router/__tests__/path-matching-test.tsx +++ b/packages/react-router/__tests__/path-matching-test.tsx @@ -1,11 +1,19 @@ import type { RouteObject } from "react-router"; import { matchRoutes } from "react-router"; -function pickPaths(routes: RouteObject[], pathname: string): string[] | null { +function pickPaths(routes: RouteObject[], pathname: string) { let matches = matchRoutes(routes, pathname); return matches && matches.map((match) => match.route.path || ""); } +function pickPathsAndParams(routes: RouteObject[], pathname: string) { + let matches = matchRoutes(routes, pathname); + return ( + matches && + matches.map((match) => ({ path: match.route.path, params: match.params })) + ); +} + describe("path matching", () => { test("root vs. dynamic", () => { let routes = [{ path: "/" }, { path: ":id" }]; @@ -287,14 +295,13 @@ describe("path matching with splats", () => { expect(matchRoutes(routes, "/prefix/abc")).toMatchInlineSnapshot(`null`); }); - test("does not support partial path matching with named parameters", () => { - let consoleWarn = jest.spyOn(console, "warn").mockImplementation(() => {}); + test("supports partial path matching with splat parameters", () => { let routes = [{ path: "/prefix*" }]; expect(matchRoutes(routes, "/prefix/abc")).toMatchInlineSnapshot(` Array [ Object { "params": Object { - "*": "abc", + "*": "/abc", }, "pathname": "/prefix/abc", "pathnameBase": "/prefix", @@ -304,20 +311,87 @@ describe("path matching with splats", () => { }, ] `); - expect(matchRoutes(routes, "/prefixabc")).toMatchInlineSnapshot(`null`); - - // Should warn on each invocation of matchRoutes - expect(consoleWarn.mock.calls).toMatchInlineSnapshot(` + expect(matchRoutes(routes, "/prefixabc")).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/*\\".", - ], + Object { + "params": Object { + "*": "abc", + }, + "pathname": "/prefixabc", + "pathnameBase": "/prefix", + "route": Object { + "path": "/prefix*", + }, + }, ] `); + }); +}); + +describe("route scoring", () => { + test.only("splat routes versus dynamic routes", () => { + let routes = [ + { path: "nested/prefix-:param/static/prefix-*" }, // Score 43 + { path: "nested/prefix-:param/static/*" }, // Score 33 + { path: "nested/prefix-:param/static" }, // Score 34 + { path: "nested/prefix-:param/*" }, // Score 22 + { path: "nested/:param/static" }, // Score 28 + { path: "nested/static" }, // Score 24 + { path: "nested/prefix-:param" }, // Score 23 + { path: "nested/prefix-*" }, // Score 22 + { path: "nested/:param" }, // Score 17 + { path: "static" }, // Score 13 + { path: "prefix-:param" }, // Score 12 + { path: "prefix-*" }, // Score 11 + { path: ":param" }, // Score 6 + { path: "*" }, // Score 1 + ]; + + // Matches are defined as [A, B, C], as in: + // "URL A should match path B with params C" + let matches: Array<[string, string, Record]> = [ + [ + "/nested/prefix-foo/static/prefix-bar/baz", + "nested/prefix-:param/static/prefix-*", + { param: "foo", "*": "bar/baz" }, + ], + [ + "/nested/prefix-foo/static/bar/baz", + "nested/prefix-:param/static/*", + { param: "foo", "*": "bar/baz" }, + ], + [ + "/nested/prefix-foo/static/bar", + "nested/prefix-:param/static/*", + { param: "foo", "*": "bar" }, + ], + [ + "/nested/prefix-foo/static", + "nested/prefix-:param/static", + { param: "foo" }, + ], + [ + "/nested/prefix-foo/bar", + "nested/prefix-:param/*", + { param: "foo", "*": "bar" }, + ], + ["/nested/foo/static", "nested/:param/static", { param: "foo" }], + ["/nested/static", "nested/static", {}], + ["/nested/prefix-foo", "nested/prefix-:param", { param: "foo" }], + ["/nested/foo", "nested/:param", { param: "foo" }], + ["/static", "static", {}], + ["/prefix-foo", "prefix-:param", { param: "foo" }], + ["/prefix-foo/bar", "prefix-*", { "*": "foo/bar" }], + ["/foo", ":param", { param: "foo" }], + ["/foo/bar/baz", "*", { "*": "foo/bar/baz" }], + ]; - consoleWarn.mockRestore(); + // Ensure order agnostic by testing route definitions forward + backwards + [...matches, ...matches.reverse()].forEach(([url, path, params]) => + expect({ + url, + matches: pickPathsAndParams(routes, url), + }).toEqual({ url, matches: [{ path, params }] }) + ); }); }); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 3bab89bc2d..b8aa641ed3 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -374,36 +374,47 @@ function rankRouteBranches(branches: RouteBranch[]): void { } const paramRe = /^:\w+$/; -const dynamicSegmentValue = 3; -const indexRouteValue = 2; +const partialParamRe = /:\w+$/; +const splatRe = /^\*$/; +const partialSplatRe = /\*$/; + +const staticSegmentValue = 10; /* /static */ +const partialDynamicSegmentValue = 9; /* /prefix-:param */ +const partialSplatValue = 8; /* /prefix-* */ +const dynamicSegmentValue = 3; /* /:param */ +const indexRouteValue = 2; /* / */ const emptySegmentValue = 1; -const staticSegmentValue = 10; -const splatPenalty = -2; -const isSplat = (s: string) => s === "*"; +const splatPenalty = -2; /* /* */ function computeScore(path: string, index: boolean | undefined): number { let segments = path.split("/"); - let initialScore = segments.length; - if (segments.some(isSplat)) { - initialScore += splatPenalty; - } + let score = segments.length; if (index) { - initialScore += indexRouteValue; + score += indexRouteValue; } - return segments - .filter((s) => !isSplat(s)) - .reduce( - (score, segment) => - score + - (paramRe.test(segment) - ? dynamicSegmentValue - : segment === "" - ? emptySegmentValue - : staticSegmentValue), - initialScore - ); + // Penalties apply globally, so only subtract them once + let splatPenaltyApplied = false; + + segments.forEach((segment) => { + if (splatRe.test(segment)) { + score += splatPenaltyApplied ? 0 : splatPenalty; + splatPenaltyApplied = true; + } else if (partialSplatRe.test(segment)) { + score += partialSplatValue; + } else if (paramRe.test(segment)) { + score += dynamicSegmentValue; + } else if (partialParamRe.test(segment)) { + score += partialDynamicSegmentValue; + } else if (segment === "") { + score += emptySegmentValue; + } else { + score += staticSegmentValue; + } + }); + + return score; } function compareIndexes(a: number[], b: number[]): number { @@ -475,23 +486,11 @@ function matchRouteBranch< * @see https://reactrouter.com/docs/en/v6/utils/generate-path */ export function generatePath( - originalPath: Path, + path: 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`); @@ -620,14 +619,6 @@ function compilePath( caseSensitive = false, end = true ): [RegExp, string[]] { - warning( - path === "*" || !path.endsWith("*") || path.endsWith("/*"), - `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(/\*$/, "/*")}".` - ); - let paramNames: string[] = []; let regexpSource = "^" + @@ -640,12 +631,18 @@ function compilePath( return "([^\\/]+)"; }); - if (path.endsWith("*")) { + if (path === "*" || path.endsWith("/*")) { + // Full splat segment paramNames.push("*"); regexpSource += path === "*" || path === "/*" ? "(.*)$" // Already matched the initial /, just match the rest : "(?:\\/(.+)|\\/*)$"; // Don't include the / in params["*"] + } else if (path.endsWith("*")) { + // Partial splat segment + paramNames.push("*"); + regexpSource += "(.*)$"; + //regexpSource += "(?:\\/(.+)|\\/*)$"; } else { regexpSource += end ? "\\/*$" // When matching to the end, ignore trailing slashes From 08b2442d187b8a8ef740f0e265aff85294a6891a Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Sep 2022 17:38:55 -0400 Subject: [PATCH 4/7] Updatew changeset --- .changeset/angry-pens-flow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/angry-pens-flow.md b/.changeset/angry-pens-flow.md index 7db066f555..27d40daeb5 100644 --- a/.changeset/angry-pens-flow.md +++ b/.changeset/angry-pens-flow.md @@ -3,4 +3,4 @@ "@remix-run/router": patch --- -fix: Align behavior of generatePath with compilePath to not support partial splat params (#9238) +fix: Permit partial splat route segment matching (i.e., `/prefix-*`) (#9238) From fe9aea6e5d8a6fba2f7d5c8b8b05ca81da0794d2 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Sep 2022 17:54:09 -0400 Subject: [PATCH 5/7] Remove leftover comment --- packages/router/utils.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index b8aa641ed3..c36619056f 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -642,7 +642,6 @@ function compilePath( // Partial splat segment paramNames.push("*"); regexpSource += "(.*)$"; - //regexpSource += "(?:\\/(.+)|\\/*)$"; } else { regexpSource += end ? "\\/*$" // When matching to the end, ignore trailing slashes From 91d3a74ff8e36d5b388ebc5192690eb3fd2cdf3f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 13 Sep 2022 10:34:09 -0400 Subject: [PATCH 6/7] Remove focused test --- packages/react-router/__tests__/path-matching-test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-router/__tests__/path-matching-test.tsx b/packages/react-router/__tests__/path-matching-test.tsx index 5e1d8cceb9..34c0db771d 100644 --- a/packages/react-router/__tests__/path-matching-test.tsx +++ b/packages/react-router/__tests__/path-matching-test.tsx @@ -329,7 +329,7 @@ describe("path matching with splats", () => { }); describe("route scoring", () => { - test.only("splat routes versus dynamic routes", () => { + test("splat routes versus dynamic routes", () => { let routes = [ { path: "nested/prefix-:param/static/prefix-*" }, // Score 43 { path: "nested/prefix-:param/static/*" }, // Score 33 From 7017172fe5fc9e41aff0aefaff20d9376646775e Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 13 Sep 2022 10:35:14 -0400 Subject: [PATCH 7/7] Add linting to workflow --- .github/workflows/test.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 12368bf489..2a1d36ec36 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -40,6 +40,9 @@ jobs: - name: Build run: yarn build + - name: Lint + run: yarn lint + - name: Test run: yarn test