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: generatePath incorrectly applying parameters #9051 #10078

Merged
merged 3 commits into from
Mar 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/old-camels-accept.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@remix-run/router": patch
"react-router": patch
---

Fix `generatePath` incorrectly applying parameters in some cases
1 change: 1 addition & 0 deletions contributors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
- ms10596
- ned-park
- noisypigeon
- Obi-Dann
- omar-moquete
- p13i
- parched
Expand Down
14 changes: 14 additions & 0 deletions packages/react-router/__tests__/generatePath-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@ describe("generatePath", () => {
);
expect(generatePath("/*", {})).toBe("/");
});
it("handles * in parameter values", () => {
expect(generatePath("/courses/:name", { name: "foo*" })).toBe(
"/courses/foo*"
);
expect(generatePath("/courses/:name", { name: "*foo" })).toBe(
"/courses/*foo"
);
expect(generatePath("/courses/:name", { name: "*f*oo*" })).toBe(
"/courses/*f*oo*"
);
expect(generatePath("/courses/:name", { name: "foo*", "*": "splat_should_not_be_added" })).toBe(
"/courses/foo*"
);
});
});

describe("with extraneous params", () => {
Expand Down
8 changes: 8 additions & 0 deletions packages/react-router/__tests__/matchPath-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,14 @@ describe("matchPath *", () => {
pathnameBase: "/",
});
});

it("resolves params containing '*' correctly", () => {
expect(matchPath("/users/:name/*", "/users/foo*/splat")).toMatchObject({
params: { name: "foo*", "*": "splat" },
pathname: "/users/foo*/splat",
pathnameBase: "/users/foo*",
});
});
});

describe("matchPath warnings", () => {
Expand Down
79 changes: 38 additions & 41 deletions packages/router/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ type _PathParam<Path extends string> =
*/
type PathParam<Path extends string> =
// check if path is just a wildcard
Path extends "*"
Path extends "*" | "/*"
? "*"
: // look for wildcard at the end of the path
Path extends `${infer Rest}/*`
Expand Down Expand Up @@ -621,7 +621,7 @@ export function generatePath<Path extends string>(
[key in PathParam<Path>]: string | null;
} = {} as any
): string {
let path = originalPath;
let path: string = originalPath;
if (path.endsWith("*") && path !== "*" && !path.endsWith("/*")) {
warning(
false,
Expand All @@ -633,49 +633,46 @@ export function generatePath<Path extends string>(
path = path.replace(/\*$/, "/*") as Path;
}

return (
path
.replace(
/^:(\w+)(\??)/g,
(_, key: PathParam<Path>, optional: string | undefined) => {
let param = params[key];
if (optional === "?") {
return param == null ? "" : param;
}
if (param == null) {
invariant(false, `Missing ":${key}" param`);
}
return param;
}
)
.replace(
/\/:(\w+)(\??)/g,
(_, key: PathParam<Path>, optional: string | undefined) => {
let param = params[key];
if (optional === "?") {
return param == null ? "" : `/${param}`;
}
if (param == null) {
invariant(false, `Missing ":${key}" param`);
}
return `/${param}`;
}
)
// Remove any optional markers from optional static segments
.replace(/\?/g, "")
.replace(/(\/?)\*/, (_, prefix, __, str) => {
// ensure `/` is added at the beginning if the path is absolute
const prefix = path.startsWith("/") ? "/" : "";

const segments = path
.split(/\/+/)
.map((segment, index, array) => {
const isLastSegment = index === array.length - 1;

// only apply the splat if it's the last segment
if (isLastSegment && segment === "*") {
const star = "*" as PathParam<Path>;
const starParam = params[star];

if (params[star] == null) {
// If no splat was provided, trim the trailing slash _unless_ it's
// the entire path
return str === "/*" ? "/" : "";
// Apply the splat
return starParam;
}

const keyMatch = segment.match(/^:(\w+)(\??)$/);
if (keyMatch) {
const [, key, optional] = keyMatch;
let param = params[key as PathParam<Path>];

if (optional === "?") {
return param == null ? "" : param;
}

// Apply the splat
return `${prefix}${params[star]}`;
})
);
if (param == null) {
invariant(false, `Missing ":${key}" param`);
}

return param;
}

// Remove any optional markers from optional static segments
return segment.replace(/\?$/g, "");
})
// Remove empty segments
.filter((segment) => !!segment);

return prefix + segments.join("/");
}

/**
Expand Down