fix(remix-react): prevent form data loss by appending submitter's value in useSubmit (<Form>)

[nrako]

Closes: (the following issue)

Currently, remix-react's <Form> through useSubmit (useSubmitImpl)
implementation, includes the submitter value by setting it on the
formData. Unfortunetly, this may override any existing inputs having the
same name than the submitter; resulting in data loss.

• Docs N/A
• Tests

Testing Strategy:

<Form>
  <input name="tasks" value="first" />
  <button name="tasks value="" type="submit">Add Task</button>
</Form>
// should submit `?tasks=first&tasks=` but submit `?tasks=`

See integration Bug-Report-Test via commit: 4187030

---

test: failing test with submitter value not appended to form data
4187030 <Nicholas Rakoto> Wed, 29 Jun 2022 22:30:59 +0200

Currently, remix-react's `<Form>` through `useSubmit` (`useSubmitImpl`) 
implementation, includes the submitter value by setting it on the
`formData`. Unfortunetly, this may override any existing inputs having the
same name than the submitter; resulting in data loss.

Instead, the submitter's value should be appended to the `formData`.

fix(remix-react): prevent form data loss by appending submitter's value
2a70250 <Nicholas Rakoto> Wed, 29 Jun 2022 22:34:39 +0200

When submitting a form the submitter's value must be appended to the
`formData` and not have it's value "set" or pre-existing data under the same
name may be overidden resulting in data loss.

This commit fixes the `useSubmit() hook (`useSubmitImpl`) and consequently
it's indirect usage through the `<Form>` component.

[nrako]

May somebody point me to a doc/thread or let me know what I can expect for the review process? (cc @MichaelDeBoey ?)

It seems this PR has been "seen", but not reviewed. CI hasn't been ran though... And unfortunately nobody seems to be assigned for reviews. If that's encouraged I can specifically mention maintainers which would seem most appropriate for review by looking at the git history.

I've also opened another PR #3613 fixing issues also touching the <Form> which will end up with conflicting code on the tests that I can of course resolve once any of them is hopefully merged – or of course a maintainers can do it too, the necessary permissions have been granted.

I'd be eager to help, both fixes are imho quite pertinent for the <Form> (or also the useSubmit in this case).

[kentcdodds]

Where does nativeEvent come from?

[nrako]

Oups, thanks. I've fixed that. The test was passing because the <form> was natively submitting with the GET method.

[nrako]

Amended again to call event.preventDefault() to only have the behavior of submit() being tested (not native form submit). I've double-checked that test and it now test what it meant to.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 8a81f7b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

[kentcdodds]

Hooray! It failed :)

[kentcdodds]

Looking into this further, this is a behavioral change that I think @ryanflorence will want to look at. I'll let him know.

[remix-cla-bot]

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

[nrako]

@kentcdodds, @ryanflorence – any chance to have a follow up on this? To be honest I don't think this really represent a behavioral change. IMHO the current implementation is the one deviating from what would be the expected behavior. I'd also say a bug which broke normal behavior.

[kiliman]

I agree, if Remix deviates from default browser behavior, then it's a bug, and this PR resolves it. I would be surprised if anyone is dependent on the buggy behavior.

I think it would be rare to see somebody using the same name in a button as for an existing input. But since the browser support this, I am +1 on the fix.