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.

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(remix-react): GET form submissions should replace current search params #4046

Merged
merged 2 commits into from Aug 26, 2022

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Aug 23, 2022

The recent work done in #3697 uncovered a previously unknown bug where we do not match default browser behavior. By default, non-JS <form method="get"> will replace any existing search parameters in the URL, but our behavior has been to append (without first resetting the params) since #814.

Default browser behavior is shown in this code sandbox. You may have to open it in a new window and load it with some existing query params, and then submit the form. I.e., https://e26g6e.sse.codesandbox.io/?junk=value

This new Remix behavior also has the side effect of no longer including the ?index in the destination URL (even though it is included in the form action). This should have no impact since GET submissions run all relevant loaders.

// routes/parent/index.tsx
<Form>
  <input name="bar" value="2" />
  <button type="submit">Submit</button>
</Form>
  • Current behavior after fix(remix-react): Preserve search string in form action when action prop is omitted #3697
    • User loads /parent?foo=1
    • Rendered HTML is <form action="/parent?index&foo=1">
    • User clicks submit
    • Browser URL navigates to /parent?index&foo=1&bar=2
    • Rendered HTML is now <form action="/parent?index&foo=1&bar=2">
  • New behavior
    • User loads /parent?foo=1
    • Rendered HTML is <form action="/parent?index&foo=1">
    • User clicks submit
    • Browser URL navigates to /parent?bar=2
    • Rendered HTML is now <form action="/parent?index&bar=2">

Closes: #4044

  • Docs
  • Tests

Testing Strategy:

  • Updated form-test integration tests

@changeset-bot
Copy link

changeset-bot bot commented Aug 23, 2022

🦋 Changeset detected

Latest commit: 2afce3f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/react Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/vercel Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

for (let [name, value] of formData) {
if (typeof value === "string") {
url.searchParams.append(name, value);
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 the underlying problem, now that our default behavior for no action was to include search params, we were just appending to them here instead of starting with an empty set on GET submissions

@jenseng
Copy link
Contributor

jenseng commented Aug 25, 2022

As a temporary workaround, you can put an action="/the/url/without/params" prop on your <Form> (or formAction on your submit button) to ensure you get a blank slate

@brophdawg11
Copy link
Contributor Author

@jenseng Yeah that should do the trick for now. You can also just do action="." which should be equivalent, since . won't include params

Copy link
Collaborator

@chaance chaance left a comment

Choose a reason for hiding this comment

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

Great explanation and repro!

@MichaelDeBoey MichaelDeBoey changed the title fix: GET form submissions should replace current search params fix(remix-react): GET form submissions should replace current search params Aug 26, 2022
@chaance chaance merged commit c22b6ac into dev Aug 26, 2022
@chaance chaance deleted the brophdawg11/form-get-params branch August 26, 2022 22:52
@MichaelDeBoey MichaelDeBoey added the awaiting release This issue has been fixed and will be released soon label Aug 26, 2022
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-c22b6ac-20220827 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@ziimakc
Copy link

ziimakc commented Sep 21, 2022

@brophdawg11 How to preserve then current search params? Mapping search params to hidden inputs is not convenient.

@brophdawg11
Copy link
Contributor Author

That would be my recommendation - I don't find it all that inconvenient:

  let [params] = useSearchParams();
  ...
  <Form>
    {Array.from(params.entries()).map(([k, v], idx) => (
      <input type="hidden" name={k} defaultValue={v} key={idx} />
    ))}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed renderer:react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants