fix(react-router-dom): fix submitter serialization (#9865)

.changeset/formdata-submitter.md

@@ -0,0 +1,5 @@
+---
+"react-router-dom": patch
+---
+
+When submitting a form from a `submitter` element, prefer the built-in `new FormData(form, submitter)` instead of the previous manual approach in modern browsers (those that support the new `submitter` parameter). For browsers that don't support it, we continue to just append the submit button's entry to the end, and we also add rudimentary support for `type="image"` buttons. If developers want full spec-compliant support for legacy browsers, they can use the `formdata-submitter-polyfill`.

package.json

@@ -40,7 +40,7 @@
   "resolutions": {
     "@types/react": "^18.0.0",
     "@types/react-dom": "^18.0.0",
-    "jsdom": "22.0.0"
+    "jsdom": "22.1.0"
   },
   "dependencies": {
     "@ampproject/filesize": "^4.3.0",
@@ -118,10 +118,10 @@
       "none": "16.2 kB"
     },
     "packages/react-router-dom/dist/react-router-dom.production.min.js": {
-      "none": "12.4 kB"
+      "none": "12.5 kB"
     },
     "packages/react-router-dom/dist/umd/react-router-dom.production.min.js": {
-      "none": "18.5 kB"
+      "none": "18.6 kB"
     }
   }
 }

packages/react-router-dom/__tests__/data-browser-router-legacy-formdata-test.tsx

@@ -0,0 +1,128 @@
+import { JSDOM } from "jsdom";
+// Drop support for the submitter parameter, as in a legacy browser. This
+// needs to be done before react-router-dom is required, since it does some
+// FormData detection.
+import "./polyfills/drop-FormData-submitter";
+import * as React from "react";
+import { render, fireEvent, screen } from "@testing-library/react";
+import "@testing-library/jest-dom";
+import {
+  Form,
+  Route,
+  RouterProvider,
+  createBrowserRouter,
+  createHashRouter,
+  createRoutesFromElements,
+} from "react-router-dom";
+
+testDomRouter("<DataBrowserRouter>", createBrowserRouter, (url) =>
+  getWindowImpl(url, false)
+);
+
+testDomRouter("<DataHashRouter>", createHashRouter, (url) =>
+  getWindowImpl(url, true)
+);
+
+function testDomRouter(
+  name: string,
+  createTestRouter: typeof createBrowserRouter | typeof createHashRouter,
+  getWindow: (initialUrl: string, isHash?: boolean) => Window
+) {
+  describe(`Router: ${name} with a legacy FormData implementation`, () => {
+    let consoleWarn: jest.SpyInstance;
+    let consoleError: jest.SpyInstance;
+
+    beforeEach(() => {
+      consoleWarn = jest.spyOn(console, "warn").mockImplementation(() => {});
+      consoleError = jest.spyOn(console, "error").mockImplementation(() => {});
+    });
+
+    afterEach(() => {
+      window.__staticRouterHydrationData = undefined;
+      consoleWarn.mockRestore();
+      consoleError.mockRestore();
+    });
+
+    describe("useSubmit/Form FormData", () => {
+      it("appends basic submitter value(s)", async () => {
+        let actionSpy = jest.fn();
+        actionSpy.mockReturnValue({});
+        async function getPayload() {
+          let formData = await actionSpy.mock.calls[
+            actionSpy.mock.calls.length - 1
+          ][0].request.formData();
+          return new URLSearchParams(formData.entries()).toString();
+        }
+
+        let router = createTestRouter(
+          createRoutesFromElements(
+            <Route path="/" action={actionSpy} element={<FormPage />} />
+          ),
+          { window: getWindow("/") }
+        );
+        render(<RouterProvider router={router} />);
+
+        function FormPage() {
+          return (
+            <>
+              <button name="tasks" value="outside" form="myform">
+                Outside
+              </button>
+              <Form id="myform" method="post">
+                <input type="text" name="tasks" defaultValue="first" />
+                <input type="text" name="tasks" defaultValue="second" />
+
+                <button name="tasks" value="">
+                  Add Task
+                </button>
+                <button value="">No Name</button>
+                <input type="image" name="tasks" alt="Add Task" />
+                <input type="image" alt="No Name" />
+
+                <input type="text" name="tasks" defaultValue="last" />
+              </Form>
+            </>
+          );
+        }
+
+        fireEvent.click(screen.getByText("Add Task"));
+        expect(await getPayload()).toEqual(
+          "tasks=first&tasks=second&tasks=last&tasks="
+        );
+
+        fireEvent.click(screen.getByText("No Name"));
+        expect(await getPayload()).toEqual(
+          "tasks=first&tasks=second&tasks=last"
+        );
+
+        fireEvent.click(screen.getByAltText("Add Task"), {
+          clientX: 1,
+          clientY: 2,
+        });
+        expect(await getPayload()).toEqual(
+          "tasks=first&tasks=second&tasks=last&tasks.x=0&tasks.y=0"
+        );
+
+        fireEvent.click(screen.getByAltText("No Name"), {
+          clientX: 1,
+          clientY: 2,
+        });
+        expect(await getPayload()).toEqual(
+          "tasks=first&tasks=second&tasks=last&x=0&y=0"
+        );
+
+        fireEvent.click(screen.getByText("Outside"));
+        expect(await getPayload()).toEqual(
+          "tasks=first&tasks=second&tasks=last&tasks=outside"
+        );
+      });
+    });
+  });
+}
+
+function getWindowImpl(initialUrl: string, isHash = false): Window {
+  // Need to use our own custom DOM in order to get a working history
+  const dom = new JSDOM(`<!DOCTYPE html>`, { url: "http://localhost/" });
+  dom.window.history.replaceState(null, "", (isHash ? "#" : "") + initialUrl);
+  return dom.window as unknown as Window;
+}

packages/react-router-dom/__tests__/data-browser-router-test.tsx

@@ -3465,6 +3465,79 @@ function testDomRouter(
         expect(formData.get("a")).toBe("1");
         expect(formData.getAll("b")).toEqual(["2", "3"]);
       });
+
+      it("includes the correct submitter value(s) in tree order", async () => {
+        let actionSpy = jest.fn();
+        actionSpy.mockReturnValue({});
+        async function getPayload() {
+          let formData = await actionSpy.mock.calls[
+            actionSpy.mock.calls.length - 1
+          ][0].request.formData();
+          return new URLSearchParams(formData.entries()).toString();
+        }
+
+        let router = createTestRouter(
+          createRoutesFromElements(
+            <Route path="/" action={actionSpy} element={<FormPage />} />
+          ),
+          { window: getWindow("/") }
+        );
+        render(<RouterProvider router={router} />);
+
+        function FormPage() {
+          return (
+            <>
+              <button name="tasks" value="outside" form="myform">
+                Outside
+              </button>
+              <Form id="myform" method="post">
+                <input type="text" name="tasks" defaultValue="first" />
+                <input type="text" name="tasks" defaultValue="second" />
+
+                <button name="tasks" value="">
+                  Add Task
+                </button>
+                <button value="">No Name</button>
+                <input type="image" name="tasks" alt="Add Task" />
+                <input type="image" alt="No Name" />
+
+                <input type="text" name="tasks" defaultValue="last" />
+              </Form>
+            </>
+          );
+        }
+
+        fireEvent.click(screen.getByText("Add Task"));
+        expect(await getPayload()).toEqual(
+          "tasks=first&tasks=second&tasks=&tasks=last"
+        );
+
+        fireEvent.click(screen.getByText("No Name"));
+        expect(await getPayload()).toEqual(
+          "tasks=first&tasks=second&tasks=last"
+        );
+
+        fireEvent.click(screen.getByAltText("Add Task"), {
+          clientX: 1,
+          clientY: 2,
+        });
+        expect(await getPayload()).toMatch(
+          "tasks=first&tasks=second&tasks.x=1&tasks.y=2&tasks=last"
+        );
+
+        fireEvent.click(screen.getByAltText("No Name"), {
+          clientX: 1,
+          clientY: 2,
+        });
+        expect(await getPayload()).toMatch(
+          "tasks=first&tasks=second&x=1&y=2&tasks=last"
+        );
+
+        fireEvent.click(screen.getByText("Outside"));
+        expect(await getPayload()).toEqual(
+          "tasks=outside&tasks=first&tasks=second&tasks=last"
+        );
+      });
     });
 
     describe("useFetcher(s)", () => {

packages/react-router-dom/__tests__/polyfills/drop-FormData-submitter.ts

@@ -0,0 +1,8 @@
+// Drop support for the submitter parameter, as in a legacy browser. This needs
+// to be a standalone module due to how jest requires things (i.e. we can't
+// just do this inline in data-browser-router-legacy-formdata-test.tsx)
+window.FormData = class FormData extends window["FormData"] {
+  constructor(form?: HTMLFormElement) {
+    super(form, undefined);
+  }
+};

packages/react-router-dom/dom.ts

@@ -126,6 +126,15 @@ export type SubmitTarget =
   | JsonValue
   | null;
 
+// One-time check for submitter support
+let formDataSupportsSubmitter = false;
+try {
+  // @ts-expect-error if FormData supports the submitter parameter, this will throw
+  new FormData(undefined, 0);
+} catch (e) {
+  formDataSupportsSubmitter = true;
+}
+
 export interface SubmitOptions {
   /**
    * The HTTP method used to submit the form. Overrides `<form method>`.
@@ -241,12 +250,22 @@ export function getFormSubmissionInfo(
       getFormEncType(form.getAttribute("enctype")) ||
       defaultEncType;
 
-    formData = new FormData(form);
-
-    // Include name + value from a <button>, appending in case the button name
-    // matches an existing input name
-    if (target.name) {
-      formData.append(target.name, target.value);
+    // Build a FormData object populated from a form and submitter
+    formData = new FormData(form, target);
+
+    // If this browser doesn't support the `FormData(el, submitter)` format,
+    // then tack on the submitter value at the end.  This is a lightweight
+    // solution that is not 100% spec compliant.  For complete support in older
+    // browsers, consider using the `formdata-submitter-polyfill` package
+    if (!formDataSupportsSubmitter) {
+      let { name, type, value } = target;
+      if (type === "image") {
+        let prefix = name ? `${name}.` : "";
+        formData.append(`${prefix}x`, "0");
+        formData.append(`${prefix}y`, "0");
+      } else if (name) {
+        formData.append(name, value);
+      }
     }
   } else if (isHtmlElement(target)) {
     throw new Error(

yarn.lock

@@ -2332,9 +2332,9 @@
     "@types/react" "*"
 
 "@types/react@*", "@types/react@^18.0.0", "@types/react@^18.0.15":
-  version "18.0.20"
-  resolved "https://registry.yarnpkg.com/@types/react/-/react-18.0.20.tgz#e4c36be3a55eb5b456ecf501bd4a00fd4fd0c9ab"
-  integrity sha512-MWul1teSPxujEHVwZl4a5HxQ9vVNsjTchVA+xRqv/VYGCuKGAU6UhfrTdF5aBefwD1BHUD8i/zq+O/vyCm/FrA==
+  version "18.2.5"
+  resolved "https://registry.yarnpkg.com/@types/react/-/react-18.2.5.tgz#f9403e1113b12b53f7edcdd9a900c10dd4b49a59"
+  integrity sha512-RuoMedzJ5AOh23Dvws13LU9jpZHIc/k90AgmK7CecAYeWmSr3553L4u5rk4sWAPBuQosfT7HmTfG4Rg5o4nGEA==
   dependencies:
     "@types/prop-types" "*"
     "@types/scheduler" "*"
@@ -6097,10 +6097,10 @@ jscodeshift@^0.13.1:
     temp "^0.8.4"
     write-file-atomic "^2.3.0"
 
-jsdom@22.0.0, jsdom@^20.0.0:
-  version "22.0.0"
-  resolved "https://registry.yarnpkg.com/jsdom/-/jsdom-22.0.0.tgz#3295c6992c70089c4b8f5cf060489fddf7ee9816"
-  integrity sha512-p5ZTEb5h+O+iU02t0GfEjAnkdYPrQSkfuTSMkMYyIoMvUNEHsbG0bHHbfXIcfTqD2UfvjQX7mmgiFsyRwGscVw==
+jsdom@22.1.0, jsdom@^20.0.0:
+  version "22.1.0"
+  resolved "https://registry.yarnpkg.com/jsdom/-/jsdom-22.1.0.tgz#0fca6d1a37fbeb7f4aac93d1090d782c56b611c8"
+  integrity sha512-/9AVW7xNbsBv6GfWho4TTNjEo9fe6Zhf9O7s0Fhhr3u+awPwAJMKwAMXnkk5vBxflqLW9hTHX/0cs+P3gW+cQw==
   dependencies:
     abab "^2.0.6"
     cssstyle "^3.0.0"