From 00aec5bffbeebf22921e5be7acf048efa3c80a96 Mon Sep 17 00:00:00 2001
From: Nicholas Rakoto <nico@nrako.com>
Date: Tue, 9 Aug 2022 16:02:10 +0200
Subject: [PATCH] fix(remix-react): prevent form data loss by appending
 submitter's value in `useSubmit` (`<Form>`) (#3611)

* test: failing test with submitter value not appended to form data

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

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.
---
 contributors.yml                    |  1 +
 integration/form-test.ts            | 35 ++++++++++++++++
 integration/hook-useSubmit-test.ts  | 64 +++++++++++++++++++++++++++++
 packages/remix-react/components.tsx |  2 +-
 4 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 integration/hook-useSubmit-test.ts

diff --git a/contributors.yml b/contributors.yml
index cbfeee6f3ad..c79afcaa2be 100644
--- a/contributors.yml
+++ b/contributors.yml
@@ -303,6 +303,7 @@
 - niwsa
 - nobeeakon
 - nordiauwu
+- nrako
 - nurul3101
 - nvh95
 - nwalters512
diff --git a/integration/form-test.ts b/integration/form-test.ts
index ab7cfd5de58..26772b65cd6 100644
--- a/integration/form-test.ts
+++ b/integration/form-test.ts
@@ -277,6 +277,29 @@ test.describe("Forms", () => {
             )
           }
         `,
+
+        "app/routes/submitter.jsx": js`
+          import { useLoaderData, Form } from "@remix-run/react";
+
+          export function loader({ request }) {
+            let url = new URL(request.url);
+            return url.searchParams.toString()
+          }
+
+          export default function() {
+            let data = useLoaderData();
+            return (
+              <Form>
+                <input type="text" name="tasks" defaultValue="first" />
+                <input type="text" name="tasks" defaultValue="second" />
+                <button type="submit" name="tasks" value="">
+                  Add Task
+                </button>
+                <pre>{data}</pre>
+              </Form>
+            )
+          }
+        `,
       },
     });
 
@@ -575,4 +598,16 @@ test.describe("Forms", () => {
       });
     });
   });
+
+  test("<Form> submits the submitter's value appended to the form data", async ({
+    page,
+  }) => {
+    let app = new PlaywrightFixture(appFixture, page);
+    await app.goto("/submitter");
+    await app.clickElement("text=Add Task");
+    await page.waitForLoadState("load");
+    expect(await app.getHtml("pre")).toBe(
+      `<pre>tasks=first&amp;tasks=second&amp;tasks=</pre>`
+    );
+  });
 });
diff --git a/integration/hook-useSubmit-test.ts b/integration/hook-useSubmit-test.ts
new file mode 100644
index 00000000000..000406a0a14
--- /dev/null
+++ b/integration/hook-useSubmit-test.ts
@@ -0,0 +1,64 @@
+import { test, expect } from "@playwright/test";
+
+import { createAppFixture, createFixture, js } from "./helpers/create-fixture";
+import type { Fixture, AppFixture } from "./helpers/create-fixture";
+import { PlaywrightFixture } from "./helpers/playwright-fixture";
+
+test.describe("`useSubmit()` returned function", () => {
+  let fixture: Fixture;
+  let appFixture: AppFixture;
+
+  test.beforeAll(async () => {
+    fixture = await createFixture({
+      files: {
+        "app/routes/index.jsx": js`
+        import { useLoaderData, useSubmit } from "@remix-run/react";
+
+        export function loader({ request }) {
+          let url = new URL(request.url);
+          return url.searchParams.toString()
+        }
+
+        export default function Index() {
+          let submit = useSubmit();
+          let handleClick = event => {
+            event.preventDefault()
+            submit(event.nativeEvent.submitter || event.currentTarget)
+          }
+          let data = useLoaderData();
+          return (
+            <form>
+              <input type="text" name="tasks" defaultValue="first" />
+              <input type="text" name="tasks" defaultValue="second" />
+
+              <button onClick={handleClick} name="tasks" value="third">
+                Prepare Third Task
+              </button>
+
+              <pre>{data}</pre>
+            </form>
+          )
+        }
+      `,
+      },
+    });
+
+    appFixture = await createAppFixture(fixture);
+  });
+
+  test.afterAll(async () => {
+    await appFixture.close();
+  });
+
+  test("submits the submitter's value appended to the form data", async ({
+    page,
+  }) => {
+    let app = new PlaywrightFixture(appFixture, page);
+    await app.goto("/");
+    await app.clickElement("text=Prepare Third Task");
+    await page.waitForLoadState("load");
+    expect(await app.getHtml("pre")).toBe(
+      `<pre>tasks=first&amp;tasks=second&amp;tasks=third</pre>`
+    );
+  });
+});
diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx
index 4627a44e030..67c68d26f27 100644
--- a/packages/remix-react/components.tsx
+++ b/packages/remix-react/components.tsx
@@ -1149,7 +1149,7 @@ export function useSubmitImpl(key?: string): SubmitFunction {
 
         // Include name + value from a <button>
         if (target.name) {
-          formData.set(target.name, target.value);
+          formData.append(target.name, target.value);
         }
       } else {
         if (isHtmlElement(target)) {