Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inadvertent support for partial dynamic parameters #9506

Merged
merged 10 commits into from
Nov 30, 2022
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
38 changes: 34 additions & 4 deletions packages/react-router/__tests__/generatePath-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
Expand Down Expand Up @@ -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("/");

Expand All @@ -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();
});
});
52 changes: 52 additions & 0 deletions packages/react-router/__tests__/path-matching-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -267,4 +267,56 @@ 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();
});
});
13 changes: 12 additions & 1 deletion packages/react-router/__tests__/useRoutes-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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[] }) => {
Expand All @@ -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: {
Expand All @@ -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: "" }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lack of search/hash in this test case was causing weird console.warn of /threeundefinedundefined

/>
</MemoryRouter>
);
});
Expand All @@ -124,6 +133,8 @@ describe("useRoutes", () => {
is null
</div>
`);

spy.mockRestore();
});

describe("warns", () => {
Expand Down
24 changes: 20 additions & 4 deletions packages/router/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -528,16 +528,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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same logic as we use in compilePath for partial splats

}

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]!}`;
})
Comment on lines -579 to +598
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generatePath does 2 separate checks here to avoid a nasty regex to differentiate :param from /thing/:param

.replace(/(\/?)\*/, (_, prefix, __, str) => {
const star = "*" as PathParam<Path>;

Expand Down Expand Up @@ -676,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 "/([^\\/]+)";
Comment on lines -721 to +739
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only match params that are at the start of a URL segment. We do not need to handle ^: here since line 693 ensures our regexpSource always has a leading slash by this point. So we just only collect params when they come immediately following a slash

});

if (path.endsWith("*")) {
Expand Down