diff --git a/.changeset/angry-pens-flow.md b/.changeset/angry-pens-flow.md new file mode 100644 index 0000000000..27d40daeb5 --- /dev/null +++ b/.changeset/angry-pens-flow.md @@ -0,0 +1,6 @@ +--- +"react-router": patch +"@remix-run/router": patch +--- + +fix: Permit partial splat route segment matching (i.e., `/prefix-*`) (#9238) 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 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 dd5e58a3af..34c0db771d 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" }]; @@ -267,4 +275,123 @@ 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("supports partial path matching with splat parameters", () => { + 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(` + Array [ + Object { + "params": Object { + "*": "abc", + }, + "pathname": "/prefixabc", + "pathnameBase": "/prefix", + "route": Object { + "path": "/prefix*", + }, + }, + ] + `); + }); +}); + +describe("route scoring", () => { + test("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" }], + ]; + + // 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 eb73f90673..c36619056f 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 { @@ -608,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 = "^" + @@ -628,12 +631,17 @@ 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 += "(.*)$"; } else { regexpSource += end ? "\\/*$" // When matching to the end, ignore trailing slashes