From 11b1ed1ba6f48f27f173815c5b5bf3637b4443e0 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 20 Dec 2022 17:23:02 -0500 Subject: [PATCH 1/6] Fix up generatePath when optional params are present --- .../__tests__/generatePath-test.tsx | 55 +++++++++++++++++++ packages/router/utils.ts | 48 +++++++++------- 2 files changed, 84 insertions(+), 19 deletions(-) diff --git a/packages/react-router/__tests__/generatePath-test.tsx b/packages/react-router/__tests__/generatePath-test.tsx index 105a2ed63f..1e1d3059ee 100644 --- a/packages/react-router/__tests__/generatePath-test.tsx +++ b/packages/react-router/__tests__/generatePath-test.tsx @@ -25,6 +25,7 @@ describe("generatePath", () => { expect(generatePath("*", { "*": "routing/grades" })).toBe( "routing/grades" ); + expect(generatePath("/*", {})).toBe("/"); }); }); @@ -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(); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 84c8b2e900..dd3b55e626 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -198,7 +198,9 @@ type _PathParam = ? _PathParam | _PathParam : // find params after `:` Path extends `:${infer Param}` - ? Param + ? Param extends `${infer Optional}?` + ? Optional + : Param : // otherwise, there aren't any params present never; @@ -629,27 +631,35 @@ export function generatePath( path = path.replace(/\*$/, "/*") as Path; } - return path - .replace(/^:(\w+)/g, (_, key: PathParam) => { - invariant(params[key] != null, `Missing ":${key}" param`); - return params[key]!; - }) - .replace(/\/:(\w+)/g, (_, key: PathParam) => { - invariant(params[key] != null, `Missing ":${key}" param`); - return `/${params[key]!}`; - }) - .replace(/(\/?)\*/, (_, prefix, __, str) => { + let leadingSlash = originalPath.startsWith("/") ? "/" : ""; + + let replaced = path.split("/").map((segment) => { + // Splats + if (segment === "*") { const star = "*" as PathParam; + return params[star] || ""; + } - if (params[star] == null) { - // If no splat was provided, trim the trailing slash _unless_ it's - // the entire path - return str === "/*" ? "/" : ""; - } + let isOptional = segment.endsWith("?"); + segment = segment.replace(/\?$/, ""); - // Apply the splat - return `${prefix}${params[star]}`; - }); + // Static segments + if (!segment.startsWith(":")) { + return segment; + } + + // Dynamic params + let key = segment.replace(/^:/, "") as PathParam; + if (params[key] != null) { + return params[key]; + } else if (!isOptional) { + invariant(false, `Missing ":${key}" param`); + } else { + return ""; + } + }); + + return leadingSlash + replaced.filter((s) => s).join("/"); } /** From 09334241fb6bc97390bfb27e89189a61c441bde9 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 20 Dec 2022 17:23:43 -0500 Subject: [PATCH 2/6] Add changeset --- .changeset/happy-ladybugs-occur.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/happy-ladybugs-occur.md diff --git a/.changeset/happy-ladybugs-occur.md b/.changeset/happy-ladybugs-occur.md new file mode 100644 index 0000000000..c6facf16db --- /dev/null +++ b/.changeset/happy-ladybugs-occur.md @@ -0,0 +1,6 @@ +--- +"react-router": patch +"@remix-run/router": patch +--- + +Fix `generatePath` when optional params are present From e27c4efca7f6004e5a7a31a6158ad5134c89fc0f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 11 Jan 2023 12:28:44 -0500 Subject: [PATCH 3/6] Revert to original approach --- packages/router/utils.ts | 68 +++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index dd3b55e626..465b5d22b8 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -631,35 +631,45 @@ export function generatePath( path = path.replace(/\*$/, "/*") as Path; } - let leadingSlash = originalPath.startsWith("/") ? "/" : ""; - - let replaced = path.split("/").map((segment) => { - // Splats - if (segment === "*") { - const star = "*" as PathParam; - return params[star] || ""; - } - - let isOptional = segment.endsWith("?"); - segment = segment.replace(/\?$/, ""); - - // Static segments - if (!segment.startsWith(":")) { - return segment; - } - - // Dynamic params - let key = segment.replace(/^:/, "") as PathParam; - if (params[key] != null) { - return params[key]; - } else if (!isOptional) { - invariant(false, `Missing ":${key}" param`); - } else { - return ""; - } - }); - - return leadingSlash + replaced.filter((s) => s).join("/"); + return ( + path + .replace( + /^:(\w+)(\??)/g, + (_, key: PathParam, optional: string | undefined) => { + let hasParam = params[key] != null; + if (optional === "?") { + return hasParam ? params[key] : ""; + } + invariant(hasParam, `Missing ":${key}" param`); + return params[key]!; + } + ) + .replace( + /\/:(\w+)(\??)/g, + (_, key: PathParam, optional: string | undefined) => { + let hasParam = params[key] != null; + if (optional === "?") { + return hasParam ? `/${params[key]}` : ""; + } + invariant(hasParam, `Missing ":${key}" param`); + return `/${params[key]!}`; + } + ) + // Remove any optional markers form optional static segments + .replace(/\?/g, "") + .replace(/(\/?)\*/, (_, prefix, __, str) => { + const star = "*" as PathParam; + + 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]}`; + }) + ); } /** From e6fabaccf9e0b2a74384e758eefcd3e661cb2018 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 11 Jan 2023 12:30:22 -0500 Subject: [PATCH 4/6] Fix typo --- packages/router/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 465b5d22b8..900cdad427 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -655,7 +655,7 @@ export function generatePath( return `/${params[key]!}`; } ) - // Remove any optional markers form optional static segments + // Remove any optional markers from optional static segments .replace(/\?/g, "") .replace(/(\/?)\*/, (_, prefix, __, str) => { const star = "*" as PathParam; From 41eaf600693bfe3580846af723a10a04ae04435e Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 11 Jan 2023 12:33:11 -0500 Subject: [PATCH 5/6] Allow null in provided params for optional params --- packages/router/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 900cdad427..6ff0d68b5c 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -234,7 +234,7 @@ export type ParamParseKey = * The parameters that were parsed from the URL path. */ export type Params = { - readonly [key in Key]: string | undefined; + readonly [key in Key]: string | null | undefined; }; /** From 7e7e3ab934aa0ff12fdbfeffab96db1f4b65e084 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 11 Jan 2023 12:42:14 -0500 Subject: [PATCH 6/6] Support null in generatePath params --- packages/router/utils.ts | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 6ff0d68b5c..88843e1ed3 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -234,7 +234,7 @@ export type ParamParseKey = * The parameters that were parsed from the URL path. */ export type Params = { - readonly [key in Key]: string | null | undefined; + readonly [key in Key]: string | undefined; }; /** @@ -616,7 +616,7 @@ function matchRouteBranch< export function generatePath( originalPath: Path, params: { - [key in PathParam]: string; + [key in PathParam]: string | null; } = {} as any ): string { let path = originalPath; @@ -636,23 +636,27 @@ export function generatePath( .replace( /^:(\w+)(\??)/g, (_, key: PathParam, optional: string | undefined) => { - let hasParam = params[key] != null; + let param = params[key]; if (optional === "?") { - return hasParam ? params[key] : ""; + return param == null ? "" : param; } - invariant(hasParam, `Missing ":${key}" param`); - return params[key]!; + if (param == null) { + invariant(false, `Missing ":${key}" param`); + } + return param; } ) .replace( /\/:(\w+)(\??)/g, (_, key: PathParam, optional: string | undefined) => { - let hasParam = params[key] != null; + let param = params[key]; if (optional === "?") { - return hasParam ? `/${params[key]}` : ""; + return param == null ? "" : `/${param}`; + } + if (param == null) { + invariant(false, `Missing ":${key}" param`); } - invariant(hasParam, `Missing ":${key}" param`); - return `/${params[key]!}`; + return `/${param}`; } ) // Remove any optional markers from optional static segments