From b696de3ddbd2b6edd8f5fd2b42ae35c0dcbeb4c8 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 26 Oct 2023 11:09:36 -0400 Subject: [PATCH 1/8] Fix issues with useFormAction/useResolvedPath for dot paths in param/splat routes --- .changeset/fix-resolve-path.md | 5 ++ .changeset/splat-form-action.md | 5 ++ .../__tests__/data-browser-router-test.tsx | 84 ++++++++++++++++++- .../__tests__/link-href-test.tsx | 2 +- packages/react-router-dom/index.tsx | 11 +-- .../__tests__/useResolvedPath-test.tsx | 28 ++++++- packages/react-router/lib/hooks.tsx | 4 +- 7 files changed, 128 insertions(+), 11 deletions(-) create mode 100644 .changeset/fix-resolve-path.md create mode 100644 .changeset/splat-form-action.md diff --git a/.changeset/fix-resolve-path.md b/.changeset/fix-resolve-path.md new file mode 100644 index 0000000000..cda6a280d7 --- /dev/null +++ b/.changeset/fix-resolve-path.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +Fix bug in `useResolvedPath` that would cause `useResolvedPath(".")` in a param or splat route to lose the params/splat portion of the URL path diff --git a/.changeset/splat-form-action.md b/.changeset/splat-form-action.md new file mode 100644 index 0000000000..4c3c7fe3aa --- /dev/null +++ b/.changeset/splat-form-action.md @@ -0,0 +1,5 @@ +--- +"react-router-dom": patch +--- + +Ensure `
` default action contains splat portion of pathname when no `action` is specified diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx index c6ffbc6354..f6d38f3900 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -2653,6 +2653,46 @@ function testDomRouter( "/foo/bar" ); }); + + it("does not include dynamic parameters from a parent layout route", async () => { + let router = createTestRouter( + createRoutesFromElements( + + }> + Param} /> + + + ), + { + window: getWindow("/foo/bar"), + } + ); + let { container } = render(); + + expect(container.querySelector("form")?.getAttribute("action")).toBe( + "/foo" + ); + }); + + it("does not include splat parameters from a parent layout route", async () => { + let router = createTestRouter( + createRoutesFromElements( + + }> + Splat} /> + + + ), + { + window: getWindow("/foo/bar/baz/qux"), + } + ); + let { container } = render(); + + expect(container.querySelector("form")?.getAttribute("action")).toBe( + "/foo" + ); + }); }); describe("index routes", () => { @@ -2895,7 +2935,7 @@ function testDomRouter( let { container } = render(); expect(container.querySelector("form")?.getAttribute("action")).toBe( - "/foo?a=1" + "/foo/bar?a=1" ); }); @@ -2915,7 +2955,7 @@ function testDomRouter( let { container } = render(); expect(container.querySelector("form")?.getAttribute("action")).toBe( - "/foo" + "/foo/bar" ); }); @@ -2935,7 +2975,45 @@ function testDomRouter( let { container } = render(); expect(container.querySelector("form")?.getAttribute("action")).toBe( - "/foo" + "/foo/bar" + ); + }); + + it("includes splat portion of path when no action is specified (inline splat)", async () => { + let router = createTestRouter( + createRoutesFromElements( + + + } /> + + + ), + { + window: getWindow("/foo/bar/baz"), + } + ); + let { container } = render(); + + expect(container.querySelector("form")?.getAttribute("action")).toBe( + "/foo/bar/baz" + ); + }); + + it("includes splat portion of path when no action is specified (nested splat)", async () => { + let router = createTestRouter( + createRoutesFromElements( + + } /> + + ), + { + window: getWindow("/foo/bar/baz"), + } + ); + let { container } = render(); + + expect(container.querySelector("form")?.getAttribute("action")).toBe( + "/foo/bar/baz" ); }); }); diff --git a/packages/react-router-dom/__tests__/link-href-test.tsx b/packages/react-router-dom/__tests__/link-href-test.tsx index 762f972ddc..51de95ac07 100644 --- a/packages/react-router-dom/__tests__/link-href-test.tsx +++ b/packages/react-router-dom/__tests__/link-href-test.tsx @@ -530,7 +530,7 @@ describe(" href", () => { }); expect(renderer.root.findByType("a").props.href).toEqual( - "/inbox/messages" + "/inbox/messages/abc" ); }); diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 9735b2bda7..fee68ba60a 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -1466,12 +1466,13 @@ export function useFormAction( let [match] = routeContext.matches.slice(-1); // Shallow clone path so we can modify it below, otherwise we modify the // object referenced by useMemo inside useResolvedPath - let path = { ...useResolvedPath(action ? action : ".", { relative }) }; + let currentPath = match.pathname.replace(/\/$/, ""); + let path = { + ...useResolvedPath(action ? action : currentPath, { relative }), + }; - // Previously we set the default action to ".". The problem with this is that - // `useResolvedPath(".")` excludes search params of the resolved URL. This is - // the intended behavior of when "." is specifically provided as - // the form action, but inconsistent w/ browsers when the action is omitted. + // If no action was specified, browsers will persist current search params + // when determining the path, so match that behavior // https://github.com/remix-run/remix/issues/927 let location = useLocation(); if (action == null) { diff --git a/packages/react-router/__tests__/useResolvedPath-test.tsx b/packages/react-router/__tests__/useResolvedPath-test.tsx index d6615e865f..1ffb82d4af 100644 --- a/packages/react-router/__tests__/useResolvedPath-test.tsx +++ b/packages/react-router/__tests__/useResolvedPath-test.tsx @@ -101,7 +101,33 @@ describe("useResolvedPath", () => { expect(renderer.toJSON()).toMatchInlineSnapshot(` 
-          {"pathname":"/users","search":"","hash":""}
+          {"pathname":"/users/mj","search":"","hash":""}
+
+ `); + }); + }); + + describe("in a param route", () => { + it("resolves . to the route path", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } + /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users/mj","search":"","hash":""}
`); }); diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 10b78505d5..381f8f0a5c 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -313,7 +313,9 @@ export function useResolvedPath( let { pathname: locationPathname } = useLocation(); let routePathnamesJson = JSON.stringify( - getPathContributingMatches(matches).map((match) => match.pathnameBase) + getPathContributingMatches(matches).map((match, idx) => + idx === matches.length - 1 ? match.pathname : match.pathnameBase + ) ); return React.useMemo( From 3ada561079666f58d9075ec3e505b661477f3cb8 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 2 Nov 2023 15:19:49 -0400 Subject: [PATCH 2/8] Update tests --- .../__tests__/useResolvedPath-test.tsx | 137 +++++++++++++++++- 1 file changed, 133 insertions(+), 4 deletions(-) diff --git a/packages/react-router/__tests__/useResolvedPath-test.tsx b/packages/react-router/__tests__/useResolvedPath-test.tsx index 1ffb82d4af..ae2cdfe9e5 100644 --- a/packages/react-router/__tests__/useResolvedPath-test.tsx +++ b/packages/react-router/__tests__/useResolvedPath-test.tsx @@ -85,7 +85,7 @@ describe("useResolvedPath", () => { }); describe("in a splat route", () => { - it("resolves . to the route path", () => { + it("resolves . to the route path (nested splat)", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { renderer = TestRenderer.create( @@ -105,18 +105,123 @@ describe("useResolvedPath", () => { `); }); + + it("resolves .. to the route path (nested splat)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
+
+ `); + }); + + it("resolves . to the route path (inline splat)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users/name/mj","search":"","hash":""}
+
+ `); + }); + + it("resolves .. to the route path (inline splat)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
+
+ `); + }); }); describe("in a param route", () => { - it("resolves . to the route path", () => { + it("resolves . to the route path (nested param)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users/mj","search":"","hash":""}
+
+ `); + }); + + it("resolves .. to the parent route (nested param)", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { renderer = TestRenderer.create( + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
+
+ `); + }); + + it("resolves . to the route path (inline param)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + } /> @@ -127,7 +232,31 @@ describe("useResolvedPath", () => { expect(renderer.toJSON()).toMatchInlineSnapshot(` 
-          {"pathname":"/users/mj","search":"","hash":""}
+          {"pathname":"/users/name/mj","search":"","hash":""}
+
+ `); + }); + + it("resolves .. to the parent route (inline param)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } + /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
`); }); From 85b30e3c26f125f38d82828677e3c7a4c528cd90 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 2 Nov 2023 15:25:22 -0400 Subject: [PATCH 3/8] Updates --- .../__tests__/data-browser-router-test.tsx | 38 +++++++++++++++++++ packages/react-router-dom/index.tsx | 5 +-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx index f6d38f3900..5f20c67ca1 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -2916,6 +2916,44 @@ function testDomRouter( "/foo/bar" ); }); + + it("includes param portion of path when no action is specified (inline splat)", async () => { + let router = createTestRouter( + createRoutesFromElements( + + + } /> + + + ), + { + window: getWindow("/foo/bar"), + } + ); + let { container } = render(); + + expect(container.querySelector("form")?.getAttribute("action")).toBe( + "/foo/bar" + ); + }); + + it("includes splat portion of path when no action is specified (nested splat)", async () => { + let router = createTestRouter( + createRoutesFromElements( + + } /> + + ), + { + window: getWindow("/foo/bar"), + } + ); + let { container } = render(); + + expect(container.querySelector("form")?.getAttribute("action")).toBe( + "/foo/bar" + ); + }); }); describe("splat routes", () => { diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index fee68ba60a..b8d8e8353f 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -1466,10 +1466,7 @@ export function useFormAction( let [match] = routeContext.matches.slice(-1); // Shallow clone path so we can modify it below, otherwise we modify the // object referenced by useMemo inside useResolvedPath - let currentPath = match.pathname.replace(/\/$/, ""); - let path = { - ...useResolvedPath(action ? action : currentPath, { relative }), - }; + let path = { ...useResolvedPath(action ? action : ".", { relative }) }; // If no action was specified, browsers will persist current search params // when determining the path, so match that behavior From 161883543bc96abd0664a2e5b7eb40a3eb9fe566 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 2 Nov 2023 15:30:25 -0400 Subject: [PATCH 4/8] Remove redundant test --- .../__tests__/data-browser-router-test.tsx | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx index 5f20c67ca1..45d8a5f92b 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -3018,26 +3018,6 @@ function testDomRouter( }); it("includes splat portion of path when no action is specified (inline splat)", async () => { - let router = createTestRouter( - createRoutesFromElements( - - - } /> - - - ), - { - window: getWindow("/foo/bar/baz"), - } - ); - let { container } = render(); - - expect(container.querySelector("form")?.getAttribute("action")).toBe( - "/foo/bar/baz" - ); - }); - - it("includes splat portion of path when no action is specified (nested splat)", async () => { let router = createTestRouter( createRoutesFromElements( From 6deb430df0a746e91c4d01a62d4f2b1e1e08d2dc Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 2 Nov 2023 15:39:07 -0400 Subject: [PATCH 5/8] Updates --- .../__tests__/useResolvedPath-test.tsx | 56 ++++++++++++++++++- packages/react-router/lib/hooks.tsx | 2 + 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/packages/react-router/__tests__/useResolvedPath-test.tsx b/packages/react-router/__tests__/useResolvedPath-test.tsx index ae2cdfe9e5..a07201f7b4 100644 --- a/packages/react-router/__tests__/useResolvedPath-test.tsx +++ b/packages/react-router/__tests__/useResolvedPath-test.tsx @@ -106,7 +106,7 @@ describe("useResolvedPath", () => { `); }); - it("resolves .. to the route path (nested splat)", () => { + it("resolves .. to the parent route path (nested splat)", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { renderer = TestRenderer.create( @@ -148,7 +148,7 @@ describe("useResolvedPath", () => { `); }); - it("resolves .. to the route path (inline splat)", () => { + it("resolves .. to the parent route path (inline splat)", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { renderer = TestRenderer.create( @@ -168,6 +168,58 @@ describe("useResolvedPath", () => { `); }); + + it("resolves . to the route path (descendant route)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + } + /> + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users/mj","search":"","hash":""}
+
+ `); + }); + + it("resolves .. to the parent route path (descendant route)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + } + /> + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
+
+ `); + }); }); describe("in a param route", () => { diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 381f8f0a5c..897c33d37b 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -312,6 +312,8 @@ export function useResolvedPath( let { matches } = React.useContext(RouteContext); let { pathname: locationPathname } = useLocation(); + // Use the full pathname for the leaf match so we include splat values + // for "." links let routePathnamesJson = JSON.stringify( getPathContributingMatches(matches).map((match, idx) => idx === matches.length - 1 ? match.pathname : match.pathnameBase From 78d55662d4708438f7e660df2d1f19b3007c2f7a Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 2 Nov 2023 15:40:02 -0400 Subject: [PATCH 6/8] Update changeset --- .changeset/fix-resolve-path.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/fix-resolve-path.md b/.changeset/fix-resolve-path.md index cda6a280d7..a7aeef9d6a 100644 --- a/.changeset/fix-resolve-path.md +++ b/.changeset/fix-resolve-path.md @@ -2,4 +2,4 @@ "react-router": patch --- -Fix bug in `useResolvedPath` that would cause `useResolvedPath(".")` in a param or splat route to lose the params/splat portion of the URL path +Fix bug in `useResolvedPath` that would cause `useResolvedPath(".")` in a splat route to lose the splat portion of the URL path From 60b47e2ec6425e2bd80fb8e082cddb5af5d1a51e Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 2 Nov 2023 15:40:50 -0400 Subject: [PATCH 7/8] Single changeset --- .changeset/fix-resolve-path.md | 2 +- .changeset/splat-form-action.md | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) delete mode 100644 .changeset/splat-form-action.md diff --git a/.changeset/fix-resolve-path.md b/.changeset/fix-resolve-path.md index a7aeef9d6a..98ab41c094 100644 --- a/.changeset/fix-resolve-path.md +++ b/.changeset/fix-resolve-path.md @@ -2,4 +2,4 @@ "react-router": patch --- -Fix bug in `useResolvedPath` that would cause `useResolvedPath(".")` in a splat route to lose the splat portion of the URL path +Fix bug in `useResolvedPath` that would cause `useResolvedPath(".")` in a splat route to lose the splat portion of the URL path. diff --git a/.changeset/splat-form-action.md b/.changeset/splat-form-action.md deleted file mode 100644 index 4c3c7fe3aa..0000000000 --- a/.changeset/splat-form-action.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"react-router-dom": patch ---- - -Ensure `` default action contains splat portion of pathname when no `action` is specified From 22a3657194e13304cabe7d0d6fa361ebb75c029f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 2 Nov 2023 15:44:01 -0400 Subject: [PATCH 8/8] Add note on bug fix --- .changeset/fix-resolve-path.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/fix-resolve-path.md b/.changeset/fix-resolve-path.md index 98ab41c094..ac876e4812 100644 --- a/.changeset/fix-resolve-path.md +++ b/.changeset/fix-resolve-path.md @@ -3,3 +3,5 @@ --- Fix bug in `useResolvedPath` that would cause `useResolvedPath(".")` in a splat route to lose the splat portion of the URL path. + +- ⚠️ This fixes a quite long-standing bug specifically for `"."` paths inside a splat route which incorrectly dropped the splat portion of the URL. If you are relative routing via `"."` inside a splat route in your application you should double check that your logic is not relying on this buggy behavior and update accordingly.