From 1a66cdfd2d1bb9be8491ee0849c6b13139a72991 Mon Sep 17 00:00:00 2001 From: Jon Jensen Date: Sat, 6 May 2023 00:57:01 -0600 Subject: [PATCH 1/6] fix(react-router-dom): fix submitter serialization Now that evergreen browsers support the submitter parameter to the FormData constructor, we can rely on that to correctly populate the form data set (i.e. ensure they appear in the right spot, ensure image buttons are encoded correctly). 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 image buttons. If developers want full support for legacy browsers, they can use the formdata-submitter-polyfill, just as this PR does for the jsdom tests. --- .changeset/neat-keys-end.md | 5 + package.json | 1 + ...ta-browser-router-legacy-formdata-test.tsx | 137 ++++++++++++++++++ .../__tests__/data-browser-router-test.tsx | 75 ++++++++++ packages/react-router-dom/dom.ts | 42 +++++- packages/react-router-dom/package.json | 1 + yarn.lock | 16 +- 7 files changed, 267 insertions(+), 10 deletions(-) create mode 100644 .changeset/neat-keys-end.md create mode 100644 packages/react-router-dom/__tests__/data-browser-router-legacy-formdata-test.tsx diff --git a/.changeset/neat-keys-end.md b/.changeset/neat-keys-end.md new file mode 100644 index 0000000000..f461641241 --- /dev/null +++ b/.changeset/neat-keys-end.md @@ -0,0 +1,5 @@ +--- +"react-router-dom": patch +--- + +fix form submitter serialization diff --git a/package.json b/package.json index 2884aece67..1076429f2d 100644 --- a/package.json +++ b/package.json @@ -70,6 +70,7 @@ "@types/semver": "^7.3.8", "@typescript-eslint/eslint-plugin": "^4.28.3", "@typescript-eslint/parser": "^4.28.3", + "@typescript/lib-dom": "npm:@types/web", "abort-controller": "^3.0.0", "babel-eslint": "^10.1.0", "babel-jest": "^29.0.3", 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..bc11d00855 --- /dev/null +++ b/packages/react-router-dom/__tests__/data-browser-router-legacy-formdata-test.tsx @@ -0,0 +1,137 @@ +import { JSDOM } from "jsdom"; + +// TODO: once https://github.com/jsdom/jsdom/pull/3496 lands, uncomment +// the following to emulate FormData behavior in older browsers. +// +// // Drop support for the submitter parameter, as in a legacy browser. This +// // needs to be done before remix is required, since it does some FormData +// // detection. +// window.FormData = class FormData extends window["FormData"] { +// constructor(form?: HTMLFormElement) { +// super(form, undefined); +// } +// }; + +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..36ffe3501a 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -1,4 +1,6 @@ import { JSDOM } from "jsdom"; +// TODO: remove this once https://github.com/jsdom/jsdom/pull/3496 lands and we upgrade jsdom +import "formdata-submitter-polyfill"; import * as React from "react"; import { act, @@ -3465,6 +3467,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/dom.ts b/packages/react-router-dom/dom.ts index 37bfdeb214..b170ce5a0c 100644 --- a/packages/react-router-dom/dom.ts +++ b/packages/react-router-dom/dom.ts @@ -241,13 +241,7 @@ export function getFormSubmissionInfo( getFormEncType(form.getAttribute("enctype")) || defaultEncType; - formData = new FormData(form); - - // Include name + value from a