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: preserve search params on unspecified <Form> action #9060

Merged
merged 7 commits into from
Aug 10, 2022
Merged

fix: preserve search params on unspecified <Form> action #9060

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
15b96e5
fix: preserve search params on unspecified form action
brophdawg11 Jul 14, 2022
3310ee4
Merge branch 'dev' into brophdawg11/form-action-params
brophdawg11 Jul 22, 2022
b6e2c7f
stengthen index tests
brophdawg11 Aug 3, 2022
6f4bc0d
Add useFormAction tests for nested routes
brophdawg11 Aug 8, 2022
3b6fc11
account for splat routes and empty action
brophdawg11 Aug 8, 2022
0ccf84c
Merge branch 'dev' into brophdawg11/form-action-params
brophdawg11 Aug 10, 2022
4e5925f
add changeset
brophdawg11 Aug 10, 2022
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
126 changes: 126 additions & 0 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 @@ -1199,6 +1199,132 @@ function testDomRouter(
`);
});

it("handles action for <Form method='get'> correctly", async () => {
let { container } = render(
<TestDataRouter window={getWindow("/?a=1#hash")} hydrationData={{}}>
<Route path="/" element={<Home />} />
</TestDataRouter>
);

function Home() {
return (
<Form method="get">
<input name="b" value="2" />
<button type="submit">Submit Form</button>
</Form>
);
}

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/?a=1#hash"
);
});

it("handles action for <Form method='get' action='.'> correctly", async () => {
let { container } = render(
<TestDataRouter window={getWindow("/?a=1")} hydrationData={{}}>
<Route path="/" element={<Home />} />
</TestDataRouter>
);

function Home() {
return (
<Form method="get" action=".">
<input name="b" value="2" />
<button type="submit">Submit Form</button>
</Form>
);
}

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

it("handles action for <Form method='post'> correctly", async () => {
let { container } = render(
<TestDataRouter window={getWindow("/?a=1#hash")} hydrationData={{}}>
<Route path="/" element={<Home />} />
</TestDataRouter>
);

function Home() {
return (
<Form method="post">
<input name="b" value="2" />
<button type="submit">Submit Form</button>
</Form>
);
}

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/?a=1#hash"
);
});

it("handles action for <Form method='post' action='.'> correctly", async () => {
let { container } = render(
<TestDataRouter window={getWindow("/?a=1")} hydrationData={{}}>
<Route path="/" element={<Home />} />
</TestDataRouter>
);

function Home() {
return (
<Form method="post" action=".">
<input name="b" value="2" />
<button type="submit">Submit Form</button>
</Form>
);
}

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

it("handles ?index param for action <Form>", async () => {
let { container } = render(
<TestDataRouter window={getWindow("/?a=1")} hydrationData={{}}>
<Route path="/">
<Route index element={<Home />} />
</Route>
</TestDataRouter>
);

function Home() {
return (
<Form method="post">
<input name="b" value="2" />
<button type="submit">Submit Form</button>
</Form>
);
}

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/?index&a=1"
);
});

it("handles ?index param for action <Form action='.'>", async () => {
let { container } = render(
<TestDataRouter window={getWindow("/?a=1")} hydrationData={{}}>
<Route path="/">
<Route index element={<Home />} />
</Route>
</TestDataRouter>
);

function Home() {
return (
<Form method="post" action=".">
<input name="b" value="2" />
<button type="submit">Submit Form</button>
</Form>
);
}

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

describe("useFetcher(s)", () => {
it("handles fetcher.load and fetcher.submit", async () => {
let count = 0;
Expand Down
15 changes: 9 additions & 6 deletions packages/react-router-dom/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,7 @@ const FormImpl = React.forwardRef<HTMLFormElement, FormImplProps>(
reloadDocument,
replace,
method = defaultMethod,
action = ".",
action,
onSubmit,
fetcherKey,
routeId,
Expand Down Expand Up @@ -851,18 +851,21 @@ function useSubmitImpl(fetcherKey?: string, routeId?: string): SubmitFunction {
);
}

export function useFormAction(action = "."): string {
export function useFormAction(action?: string): string {
let routeContext = React.useContext(RouteContext);
invariant(routeContext, "useFormAction must be used inside a RouteContext");

let location = useLocation();
let [match] = routeContext.matches.slice(-1);
let { pathname, search } = useResolvedPath(action);
let path = useResolvedPath(action || location);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should still resolve the path based on "." if the action is undefined, no? Does that make a difference here? My reading of resolveTo makes me believe that it would if we were in a nested route situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that puts us back in the same scenario though where we lose search params? Down inside useResolvedPath -> resolveTo -> parsePath we end up with a Path of { pathname: '.' } without any search or hash. By passing the current location when no action is specified we preserve those throughout?

Just to be safe, I added a few tests for the nested route scenario to confirm they're handled - and they break if we change to let path = useResolvedPath(action || '.')


if (action === "." && match.route.index) {
search = search ? search.replace(/^\?/, "?index&") : "?index";
if ((!action || action === ".") && match.route.index) {
path.search = path.search
? path.search.replace(/^\?/, "?index&")
: "?index";
}

return pathname + search;
return createPath(path);
}

function createFetcherForm(fetcherKey: string, routeId: string) {
Expand Down
24 changes: 22 additions & 2 deletions packages/router/__tests__/router-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3467,7 +3467,7 @@ describe("a router", () => {
expect(t.router.state.navigation.formData).toBeUndefined();
});

it("merges URLSearchParams for formMethod=get", async () => {
it("does not preserve existing 'action' URLSearchParams for formMethod='get'", async () => {
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 was a previous misunderstanding on my part - browser do indeed blow away query params on GET submissions. I thought it would just append them

let t = setup({
routes: TASK_ROUTES,
initialEntries: ["/"],
Expand All @@ -3479,12 +3479,32 @@ describe("a router", () => {
expect(t.router.state.navigation.state).toBe("loading");
expect(t.router.state.navigation.location).toMatchObject({
pathname: "/tasks",
search: "?key=1&key=2",
search: "?key=2",
});
expect(t.router.state.navigation.formMethod).toBeUndefined();
expect(t.router.state.navigation.formData).toBeUndefined();
});

it("preserves existing 'action' URLSearchParams for formMethod='post'", async () => {
let t = setup({
routes: TASK_ROUTES,
initialEntries: ["/"],
});
await t.navigate("/tasks?key=1", {
formMethod: "post",
formData: createFormData({ key: "2" }),
});
expect(t.router.state.navigation.state).toBe("submitting");
expect(t.router.state.navigation.location).toMatchObject({
pathname: "/tasks",
search: "?key=1",
});
expect(t.router.state.navigation.formMethod).toBe("post");
expect(t.router.state.navigation.formData).toEqual(
createFormData({ key: "2" })
);
});

it("returns a 400 error if binary data is attempted to be submitted using formMethod=GET", async () => {
let t = setup({
routes: TASK_ROUTES,
Expand Down
18 changes: 5 additions & 13 deletions packages/router/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2029,12 +2029,9 @@ function normalizeNavigateOptions(

// Flatten submission onto URLSearchParams for GET submissions
let parsedPath = parsePath(path);
let searchParams;
try {
searchParams = convertFormDataToSearchParams(
opts.formData,
parsedPath.search
);
let searchParams = convertFormDataToSearchParams(opts.formData);
parsedPath.search = `?${searchParams}`;
} catch (e) {
return {
path,
Expand All @@ -2046,9 +2043,7 @@ function normalizeNavigateOptions(
};
}

return {
path: createPath({ ...parsedPath, search: `?${searchParams}` }),
};
return { path: createPath(parsedPath) };
}

function getLoaderRedirect(
Expand Down Expand Up @@ -2331,11 +2326,8 @@ function createRequest(
return new Request(url, init);
}

function convertFormDataToSearchParams(
formData: FormData,
initialSearchParams?: string
): URLSearchParams {
let searchParams = new URLSearchParams(initialSearchParams);
function convertFormDataToSearchParams(formData: FormData): URLSearchParams {
let searchParams = new URLSearchParams();

for (let [key, value] of formData.entries()) {
invariant(
Expand Down