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.

Sign up for GitHub

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

Jump to bottom

Fix issues with useFormAction/useResolvedPath for dot paths in param/splat routes #10983

Merged
merged 8 commits into from
Nov 3, 2023
+322 −10
Merged

Fix issues with useFormAction/useResolvedPath for dot paths in param/splat routes #10983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b696de3
Fix issues with useFormAction/useResolvedPath for dot paths in param/…
brophdawg11 Oct 26, 2023
3ada561
Update tests
brophdawg11 Nov 2, 2023
85b30e3
Updates
brophdawg11 Nov 2, 2023
1618835
Remove redundant test
brophdawg11 Nov 2, 2023
6deb430
Updates
brophdawg11 Nov 2, 2023
78d5566
Update changeset
brophdawg11 Nov 2, 2023
60b47e2
Single changeset
brophdawg11 Nov 2, 2023
22a3657
Add note on bug fix
brophdawg11 Nov 2, 2023
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
7 changes: 7 additions & 0 deletions .changeset/fix-resolve-path.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
102 changes: 99 additions & 3 deletions packages/react-router-dom/__tests__/data-browser-router-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2653,6 +2653,46 @@ function testDomRouter(
"/foo/bar"
);
});

it("does not include dynamic parameters from a parent layout route", async () => {
let router = createTestRouter(
createRoutesFromElements(
<Route path="/">
<Route path="foo" element={<ActionEmptyComponent />}>
<Route path=":param" element={<h1>Param</h1>} />
</Route>
</Route>
),
{
window: getWindow("/foo/bar"),
}
);
let { container } = render(<RouterProvider router={router} />);

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo"
);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New tests we didn't have before to ensure that the useResolvedPath changes don't make us accidentally pick up child route portions


it("does not include splat parameters from a parent layout route", async () => {
let router = createTestRouter(
createRoutesFromElements(
<Route path="/">
<Route path="foo" element={<ActionEmptyComponent />}>
<Route path="*" element={<h1>Splat</h1>} />
</Route>
</Route>
),
{
window: getWindow("/foo/bar/baz/qux"),
}
);
let { container } = render(<RouterProvider router={router} />);

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo"
);
});
});

describe("index routes", () => {
Expand Down Expand Up @@ -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(
<Route path="/">
<Route path="foo">
<Route path=":param" element={<NoActionComponent />} />
</Route>
</Route>
),
{
window: getWindow("/foo/bar"),
}
);
let { container } = render(<RouterProvider router={router} />);

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(
<Route path="/">
<Route path="foo/:param" element={<NoActionComponent />} />
</Route>
),
{
window: getWindow("/foo/bar"),
}
);
let { container } = render(<RouterProvider router={router} />);

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo/bar"
);
});
});

describe("splat routes", () => {
Expand All @@ -2895,7 +2973,7 @@ function testDomRouter(
let { container } = render(<RouterProvider router={router} />);

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo?a=1"
"/foo/bar?a=1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This existing test was asserting the buggy behavior. When no action is specified it should inherit the current path including the splat portion

);
});

Expand All @@ -2915,7 +2993,7 @@ function testDomRouter(
let { container } = render(<RouterProvider router={router} />);

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo"
"/foo/bar"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This existing test was asserting the buggy behavior. When action="." is specified it should inherit the current path including the splat portion

);
});

Expand All @@ -2935,7 +3013,25 @@ function testDomRouter(
let { container } = render(<RouterProvider router={router} />);

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(
<Route path="/">
<Route path="foo/*" element={<NoActionComponent />} />
</Route>
),
{
window: getWindow("/foo/bar/baz"),
}
);
let { container } = render(<RouterProvider router={router} />);

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo/bar/baz"
);
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/react-router-dom/__tests__/link-href-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ describe("<Link> href", () => {
});

expect(renderer.root.findByType("a").props.href).toEqual(
"/inbox/messages"
"/inbox/messages/abc"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above - this asserted the buggy behavior

);
});

Expand Down
6 changes: 2 additions & 4 deletions packages/react-router-dom/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1468,10 +1468,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) {
Expand Down
209 changes: 208 additions & 1 deletion packages/react-router/__tests__/useResolvedPath-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -99,6 +99,213 @@ describe("useResolvedPath", () => {
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users/mj","search":"","hash":""}
</pre>
`);
});

it("resolves .. to the parent route path (nested splat)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/mj"]}>
<Routes>
<Route path="/users">
<Route path="*" element={<ShowResolvedPath path=".." />} />
</Route>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users","search":"","hash":""}
</pre>
`);
});

it("resolves . to the route path (inline splat)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/name/mj"]}>
<Routes>
<Route path="/users">
<Route path="name/*" element={<ShowResolvedPath path="." />} />
</Route>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users/name/mj","search":"","hash":""}
</pre>
`);
});

it("resolves .. to the parent route path (inline splat)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/name/mj"]}>
<Routes>
<Route path="/users">
<Route path="name/*" element={<ShowResolvedPath path=".." />} />
</Route>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users","search":"","hash":""}
</pre>
`);
});

it("resolves . to the route path (descendant route)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/mj"]}>
<Routes>
<Route
path="/users/*"
element={
<Routes>
<Route path="mj" element={<ShowResolvedPath path="." />} />
</Routes>
}
/>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users/mj","search":"","hash":""}
</pre>
`);
});

it("resolves .. to the parent route path (descendant route)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/mj"]}>
<Routes>
<Route
path="/users/*"
element={
<Routes>
<Route path="mj" element={<ShowResolvedPath path=".." />} />
</Routes>
}
/>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users","search":"","hash":""}
</pre>
`);
});
});

describe("in a param route", () => {
it("resolves . to the route path (nested param)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/mj"]}>
<Routes>
<Route path="/users">
<Route path=":name" element={<ShowResolvedPath path="." />} />
</Route>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users/mj","search":"","hash":""}
</pre>
`);
});

it("resolves .. to the parent route (nested param)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/mj"]}>
<Routes>
<Route path="/users">
<Route path=":name" element={<ShowResolvedPath path=".." />} />
</Route>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users","search":"","hash":""}
</pre>
`);
});

it("resolves . to the route path (inline param)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/name/mj"]}>
<Routes>
<Route path="/users">
<Route
path="name/:name"
element={<ShowResolvedPath path="." />}
/>
</Route>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users/name/mj","search":"","hash":""}
</pre>
`);
});

it("resolves .. to the parent route (inline param)", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/users/name/mj"]}>
<Routes>
<Route path="/users">
<Route
path="name/:name"
element={<ShowResolvedPath path=".." />}
/>
</Route>
</Routes>
</MemoryRouter>
);
});

expect(renderer.toJSON()).toMatchInlineSnapshot(`
<pre>
{"pathname":"/users","search":"","hash":""}
Expand Down
Loading