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 up generatePath when optional params are present #9764

Merged
merged 6 commits into from
Jan 11, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/happy-ladybugs-occur.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"react-router": patch
"@remix-run/router": patch
---

Fix `generatePath` when optional params are present
55 changes: 55 additions & 0 deletions packages/react-router/__tests__/generatePath-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ describe("generatePath", () => {
expect(generatePath("*", { "*": "routing/grades" })).toBe(
"routing/grades"
);
expect(generatePath("/*", {})).toBe("/");
});
});

Expand All @@ -49,6 +50,60 @@ describe("generatePath", () => {
});
});

describe("with optional params", () => {
it("adds optional dynamic params where appropriate", () => {
let path = "/:one?/:two?/:three?";
expect(generatePath(path, { one: "uno" })).toBe("/uno");
expect(generatePath(path, { one: "uno", two: "dos" })).toBe("/uno/dos");
expect(
generatePath(path, {
one: "uno",
two: "dos",
three: "tres",
})
).toBe("/uno/dos/tres");
expect(generatePath(path, { one: "uno", three: "tres" })).toBe(
"/uno/tres"
);
expect(generatePath(path, { two: "dos" })).toBe("/dos");
expect(generatePath(path, { two: "dos", three: "tres" })).toBe(
"/dos/tres"
);
});

it("strips optional aspects of static segments", () => {
expect(generatePath("/one?/two?/:three?", {})).toBe("/one/two");
expect(generatePath("/one?/two?/:three?", { three: "tres" })).toBe(
"/one/two/tres"
);
});

it("handles intermixed segments", () => {
let path = "/one?/:two?/three/:four/*";
expect(generatePath(path, { four: "cuatro" })).toBe("/one/three/cuatro");
expect(
generatePath(path, {
two: "dos",
four: "cuatro",
})
).toBe("/one/dos/three/cuatro");
expect(
generatePath(path, {
two: "dos",
four: "cuatro",
"*": "splat",
})
).toBe("/one/dos/three/cuatro/splat");
expect(
generatePath(path, {
two: "dos",
four: "cuatro",
"*": "splat/and/then/some",
})
).toBe("/one/dos/three/cuatro/splat/and/then/some");
});
});

it("throws only on on missing named parameters, but not missing splat params", () => {
expect(() => generatePath(":foo")).toThrow();
expect(() => generatePath("/:foo")).toThrow();
Expand Down
70 changes: 47 additions & 23 deletions packages/router/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,9 @@ type _PathParam<Path extends string> =
? _PathParam<L> | _PathParam<R>
: // find params after `:`
Path extends `:${infer Param}`
? Param
? Param extends `${infer Optional}?`
? Optional
: Param
: // otherwise, there aren't any params present
never;

Expand Down Expand Up @@ -614,7 +616,7 @@ function matchRouteBranch<
export function generatePath<Path extends string>(
originalPath: Path,
params: {
[key in PathParam<Path>]: string;
[key in PathParam<Path>]: string | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our types still require you pass the key so accept null for optional params.

} = {} as any
): string {
let path = originalPath;
Expand All @@ -629,27 +631,49 @@ export function generatePath<Path extends string>(
path = path.replace(/\*$/, "/*") as Path;
}

return 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]!}`;
})
.replace(/(\/?)\*/, (_, prefix, __, str) => {
const star = "*" as PathParam<Path>;

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 `${prefix}${params[star]}`;
});
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) => {
const star = "*" as PathParam<Path>;

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 `${prefix}${params[star]}`;
})
);
}

/**
Expand Down