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): fix <Form> submit to not break formMethod functionality #3613

Closed
wants to merge 2 commits into from

Conversation

nrako
Copy link
Contributor

@nrako nrako commented Jun 29, 2022

Closes: #2162 (supersede) – credits to @mdoury for his original PR

the <Form> component – unlike native <form> - does not respect the formmethod attribute set
on the submitter (the button submitting the form).

  • Docs N/A
  • Tests

Testing Strategy:

<Form method="post">
  <input name="eat" value="pizza" />
  <button type="submit"> Submit with GET </button>
</Form>
// should submit with a GET to `/?eat=pizza` but instead do a POST

See integration Bug-Report-Test via commit: 37b0140


test: failing test with <Form> not respecting formMethod
06333c1 <Nicholas Rakoto> Thu, 30 Jun 2022 00:33:15 +0200

Bug report integration test demonstrating how the `<Form>` component – 
unlike native `<form>` - does not respect the `formmethod` attribute set on
the submitter (the `<button>` submitting the form).

fix(remix-react): submit <Form> w/method set by submitter formmethod
bfd1f88 <Nicholas Rakoto> Thu, 30 Jun 2022 00:32:22 +0200

Fix `<Form>` component by not passing the form's `method` into the
`options` when calling the `submit(target, options)` function (created with
the `useSubmit()` hook). This ensures that the option does not take 
precedence on any `formethod` set on the submitter which otherwise break the
`formmethod` functionality.

Since #3094 and #3094 the `<Form>` passes the "submitter" (if any) as the
`target` to the `submit(target, options)` (`useSubmitImpl`) which will
attempt to read the `formmethod`, `formaction` and `formenctype` attributes
on the target. Therefore, the responsability to infer which method should be
used should solely be on `useSubmitImpl` for the given
`target`.

Co-authored-by: Maxime Doury <[email protected]>

Bug report integration test demonstrating how the `<Form>` component –
unlike native `<form>` - does not respect the `formmethod` attribute set
on the submitter (the `<button>` submitting the form).
@MichaelDeBoey MichaelDeBoey changed the title fix(remix-react): fix <Form> submit to not break formmethod functionality fix(remix-react): fix <Form> submit to not break formMethod functionality Jun 29, 2022
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Could you maybe add the other tests from #2162 as well please?

Fix `<Form>` component by not passing the form's `method` into the
`options` when calling the `submit(target, options)` function (created
with the `useSubmit()` hook). This ensures that the option does not take
precedence on any `formethod` set on the submitter which otherwise break
the `formmethod` functionality.

Since remix-run#3094 and remix-run#3094 the `<Form>` passes the "submitter" (if any) as
the `target` to the `submit(target, options)` (`useSubmitImpl`) which
will attempt to read the `formmethod`, `formaction` and `formenctype`
attributes on the target. Therefore, the responsability to infer which
method should be used should solely be on `useSubmitImpl` for the given
`target`.

Co-authored-by: Maxime Doury <[email protected]>
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 30, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@nrako
Copy link
Contributor Author

nrako commented Jun 30, 2022

@MichaelDeBoey – could you please be a bit more explicit about the "other tests". The tests in this PR specifically cover what is fixed, which is to ensure that the formethod on a submit button is not ignored. The only other test I see in the PR is one that more or less verify that a <Form> submit with the method specified which I believe is already covered by many other existing tests.

Note: I've refactored the tests a bit, fixed the failing test, put the tests under a new describe and split the assertions in two different tests for both POST and GET.

@evanstern
Copy link

Any movement on this? Is there any way I can help get this across the finish line?

@MichaelDeBoey MichaelDeBoey removed their request for review July 21, 2022 14:05
@MichaelDeBoey
Copy link
Member

@nrako The tests in #2162 have split tests for using the form method attribute, the button formmethod attribute & combined, you only have tests for the combined case

@nrako
Copy link
Contributor Author

nrako commented Aug 2, 2022

@MichaelDeBoey – well yes, but as I mentioned:

The only other test I see in the PR is one that more or less verify that a <Form> submit with the method specified which I believe is already covered by many other existing tests.

I personally don't believe that it is the responsibility of this PR to add tests coverage for something which is unrelated to what this PR fix and is also already covered by other tests.

But feel free to add them, Remix's maintainers have edit privilege on my PR/branch. If I get a quick reply that can change my mind I might try to follow up on it but to get expectation clear, I'll soon be off for vacations.

@evanstern
Copy link

If someone can give me some instructions on how I can make these changes, I'd be happy to make them so this can get merged. This is functionality that I desperately need for my project.

@kevlened
Copy link
Contributor

I opened a PR at #4053 that adds the tests from #2162

@MichaelDeBoey
Copy link
Member

Closing this one in favor of #4053

@MichaelDeBoey MichaelDeBoey added the duplicate This issue or pull request already exists label Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed duplicate This issue or pull request already exists feat:forms renderer:react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants