From f63774c3843e2f25b6ca5b6f4882ebd4db705603 Mon Sep 17 00:00:00 2001 From: Jon Jensen Date: Wed, 14 Jun 2023 15:16:23 -0600 Subject: [PATCH] fix(react-router-dom): fix submitter serialization (#9865) --- .changeset/formdata-submitter.md | 5 + package.json | 6 +- ...ta-browser-router-legacy-formdata-test.tsx | 128 ++++++++++++++++++ .../__tests__/data-browser-router-test.tsx | 73 ++++++++++ .../polyfills/drop-FormData-submitter.ts | 8 ++ packages/react-router-dom/dom.ts | 31 ++++- yarn.lock | 14 +- 7 files changed, 249 insertions(+), 16 deletions(-) create mode 100644 .changeset/formdata-submitter.md create mode 100644 packages/react-router-dom/__tests__/data-browser-router-legacy-formdata-test.tsx create mode 100644 packages/react-router-dom/__tests__/polyfills/drop-FormData-submitter.ts diff --git a/.changeset/formdata-submitter.md b/.changeset/formdata-submitter.md new file mode 100644 index 0000000000..062de9b49b --- /dev/null +++ b/.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`. diff --git a/package.json b/package.json index c2c1761b04..4d1c08969e 100644 --- a/package.json +++ b/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" } } } diff --git a/packages/react-router-dom/__tests__/data-browser-router-legacy-formdata-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-legacy-formdata-test.tsx new file mode 100644 index 0000000000..1c524d6937 --- /dev/null +++ b/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("", createBrowserRouter, (url) => + getWindowImpl(url, false) +); + +testDomRouter("", 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( + } /> + ), + { window: getWindow("/") } + ); + render(); + + function FormPage() { + return ( + <> + +
+ + + + + + + + + +
+ + ); + } + + 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(``, { url: "http://localhost/" }); + dom.window.history.replaceState(null, "", (isHash ? "#" : "") + initialUrl); + return dom.window as unknown as Window; +} diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx index ea7b693665..96d58d30d5 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/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( + } /> + ), + { window: getWindow("/") } + ); + render(); + + function FormPage() { + return ( + <> + +
+ + + + + + + + + +
+ + ); + } + + 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)", () => { diff --git a/packages/react-router-dom/__tests__/polyfills/drop-FormData-submitter.ts b/packages/react-router-dom/__tests__/polyfills/drop-FormData-submitter.ts new file mode 100644 index 0000000000..c3c93e0065 --- /dev/null +++ b/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); + } +}; diff --git a/packages/react-router-dom/dom.ts b/packages/react-router-dom/dom.ts index 37bfdeb214..cf0700edeb 100644 --- a/packages/react-router-dom/dom.ts +++ b/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 `
`. @@ -241,12 +250,22 @@ export function getFormSubmissionInfo( getFormEncType(form.getAttribute("enctype")) || defaultEncType; - formData = new FormData(form); - - // Include name + value from a