From fe066bd4641a17142e73569b116c0236d8c9e1d7 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 3 Nov 2023 10:41:03 -0400 Subject: [PATCH] Fix issues with useFormAction/useResolvedPath for dot paths in param/splat routes (#10983) --- .changeset/fix-resolve-path.md | 7 + .../__tests__/data-browser-router-test.tsx | 102 ++++++++- .../__tests__/link-href-test.tsx | 2 +- packages/react-router-dom/index.tsx | 6 +- .../__tests__/useResolvedPath-test.tsx | 209 +++++++++++++++++- packages/react-router/lib/hooks.tsx | 6 +- 6 files changed, 322 insertions(+), 10 deletions(-) create mode 100644 .changeset/fix-resolve-path.md diff --git a/.changeset/fix-resolve-path.md b/.changeset/fix-resolve-path.md new file mode 100644 index 0000000000..ac876e4812 --- /dev/null +++ b/.changeset/fix-resolve-path.md @@ -0,0 +1,7 @@ +--- +"react-router": patch +--- + +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. 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..45d8a5f92b 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", () => { @@ -2876,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", () => { @@ -2895,7 +2973,7 @@ function testDomRouter( let { container } = render(); expect(container.querySelector("form")?.getAttribute("action")).toBe( - "/foo?a=1" + "/foo/bar?a=1" ); }); @@ -2915,7 +2993,7 @@ function testDomRouter( let { container } = render(); expect(container.querySelector("form")?.getAttribute("action")).toBe( - "/foo" + "/foo/bar" ); }); @@ -2935,7 +3013,25 @@ 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" ); }); }); 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 1031c5b65c..ec1bc769d2 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -1478,10 +1478,8 @@ export function useFormAction( // object referenced by useMemo inside useResolvedPath let path = { ...useResolvedPath(action ? action : ".", { 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..a07201f7b4 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( @@ -99,6 +99,213 @@ describe("useResolvedPath", () => { ); }); + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users/mj","search":"","hash":""}
+
+ `); + }); + + it("resolves .. to the parent 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 parent route path (inline splat)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
+
+ `); + }); + + 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", () => { + 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( + + + + } + /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"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":""}
diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx
index 10b78505d5..897c33d37b 100644
--- a/packages/react-router/lib/hooks.tsx
+++ b/packages/react-router/lib/hooks.tsx
@@ -312,8 +312,12 @@ 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) => match.pathnameBase)
+    getPathContributingMatches(matches).map((match, idx) =>
+      idx === matches.length - 1 ? match.pathname : match.pathnameBase
+    )
   );
 
   return React.useMemo(