-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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.
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
fix: preserve search params on unspecified <Form> action #9060
Changes from 1 commit
15b96e5
3310ee4
b6e2c7f
6f4bc0d
3b6fc11
0ccf84c
4e5925f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1117,6 +1117,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("/"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But only keeps pathname for |
||
}); | ||
|
||
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("/")} 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" | ||
); | ||
}); | ||
|
||
it("handles ?index param for action <Form action='.'>", async () => { | ||
let { container } = render( | ||
<TestDataRouter window={getWindow("/")} 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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -566,7 +566,7 @@ const FormImpl = React.forwardRef<HTMLFormElement, FormImplProps>( | |
{ | ||
replace, | ||
method = defaultMethod, | ||
action = ".", | ||
action, | ||
onSubmit, | ||
fetcherKey, | ||
routeId, | ||
|
@@ -805,18 +805,21 @@ function useSubmitImpl(fetcherKey?: string, routeId?: string): SubmitFunction { | |
); | ||
} | ||
|
||
export function useFormAction(action = "."): string { | ||
export function useFormAction(action?: string): string { | ||
let routeContext = React.useContext(UNSAFE_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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should still resolve the path based on There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
|
||
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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3320,7 +3320,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 () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: ["/"], | ||
|
@@ -3332,12 +3332,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, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserves everything if no action is specified