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

jenseng · 2023-01-10T01:11:25Z

Now that evergreen browsers support the submitter parameter to the FormData constructor, we can use it to correctly populate the form data set (i.e. ensure submit buttons 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.

References: remix-run/remix#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-form-data-set

changeset-bot · 2023-01-10T01:11:28Z

🦋 Changeset detected

Latest commit: c297d9b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
react-router-dom	Patch
react-router	Patch
react-router-dom-v5-compat	Patch
react-router-native	Patch

Not sure what this means? Click here to learn what changesets are.

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

jenseng · 2023-01-12T17:25:17Z

Seeing if I can get the ball rolling on adding submitter to the FormData constructor in the spec, then evergreen browsers could support this natively and we'd only need to polyfill for older ones 🤞

brophdawg11 · 2023-01-24T22:35:22Z

ok talked to Michael and Ryan and gonna aim to get this approach merged - I'll try to take a deeper look through it this week 👍

jenseng · 2023-01-24T22:43:01Z

Nice! Looks like momentum is building on getting the FormData constructor updated to include the submitter ... I already have basic implementations in Blink/Firefox/WebKit/jsdom, and so far the Blink/Firefox implementors have expressed interest 🥳

Assuming that's the direction things go, I wonder if I should extract this into a simple/standalone polyfill that Remix could use? Then the actual react-router change is just a couple lines, wdyt?

brophdawg11 · 2023-01-24T22:50:20Z

Awesome news! Yeah personally I'd love to not have to have Remix/RR worry about this at all and let the browser handle it if supported, and let users polyfill otherwise (if they care about serialization order). Is that what you're envisioning?

jenseng · 2023-01-24T23:03:06Z

This patch (and an equivalent standalone polyfill) fixes more than just the order, but that could be a good starting point, e.g.

formData = new FormData(form, target);

// For older browsers that don't support the FormData constructor submitter parameter,
// be sure to set a value. To more robustly support old browsers in your react-router/,
// Remix app, consider using the `formdata-submitter-polyfill` package
if (target.name && target.type === "submit" && formData.get(target.name) !== target.value) {
  formData.append(target.name, target.value);
}

The nice thing about rolling the polyfill/fix into RR is it also 1. fixes the values for image buttons (i.e. you get x/y coordinates) and 2. it fixes the double-submitter bug in old Safari

MichaelDeBoey

I think we can remove both polyfills & update jsdom version as jsdom/jsdom#3480 & jsdom/jsdom#3484 are both merged & released

jenseng · 2023-01-27T01:08:50Z

The polyfills are no longer needed when using [email protected], but jest still requires [email protected] ... I'll see if I can move the needle there

jenseng · 2023-01-30T16:23:27Z

FormData submitter has landed in the spec and the WebKit implementation just got merged, Chromium and Gecko should be close behind 🤞

I'll change this PR to a lighter weight approach (e.g. fall back to adding the entry explicitly, or use this polyfill), but I'll need to get jsdom updated (again 😆 ) before I do that. So between getting jest and jsdom updated, this PR is kind of in a holding pattern, so I'll switch it to a draft in the meantime.

brophdawg11 · 2023-02-01T15:06:29Z

@jenseng oh dang, nice work! Would you mind re-assigning me when things get moving again here to get it on my radar?

github-actions · 2023-04-17T20:09:53Z

This PR has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this PR will be automatically closed.

jenseng · 2023-04-18T21:59:29Z

I've been in a slight holding pattern with jest and jsdom. Once a new version of jsdom is released with this fix, jest will be able to upgrade to the latest jsdom, which includes other fixes that will let us remove the various polyfills.

I also have a PR to add submitter support to jsdom, though we could potentially move along without that, if we're open to idea of robustly polyfilling it for old browsers and jsdom ... see https://github.com/jenseng/formdata-submitter-polyfill

jenseng · 2023-05-06T20:35:17Z

I've got an update just about ready for this PR, just want to get #10453 merged first as it will simplify stuff a bit :)

jenseng · 2023-05-25T23:38:15Z

Ok @brophdawg11, this should be good to go 🤞 ... I don't have permission to assign issues, so pinging you yet again 🙂

I tried to keep the change as minimal as possible, though I still had to bump up the package sizes a tad

jenseng · 2023-05-25T23:41:49Z

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",


typescript 4.8 ships with an older dom lib, this gets us on the latest so that TS knows about the submitter

Does this go away if we upgrade to TS 5? If so I can do that against dev and then we can remove

Looks like none of the 5.0.x releases have it, but it's included in 5.1.3

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

jenseng · 2023-05-26T16:36:19Z

Two slight variations we could consider:

Keep the PR mostly as-is, but don't bother with the image button logic. Then it works essentially the same as before if you're on old browser. If people want 100% correct submitter behavior for old browsers, they can use a polyfill.
Use this polyfill. This would simplify the code, reduce the bundle size, and make the tests redundant, but then react-router-dom would have a third-party dependency

jenseng · 2023-05-27T17:45:54Z

New jsdom just landed w/ submitter support, so I just pushed a commit to simplify tests a tiny bit

brophdawg11 · 2023-06-06T21:21:14Z

Thanks @jenseng! I inlined the buildFormData method since it only applies to the button/image fork currently. Going to look into upgrading to Ts 5 against dev and hopefully remove the need to hijack the lib-dom types

jenseng · 2023-06-09T23:21:41Z

@brophdawg11 here's a PR to upgrade to the latest typescript

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.

this was previously needed to get the latest DOM types also bump packages sizes

jenseng · 2023-06-14T15:10:21Z

rebased and removed @types/web since [email protected] gives us the latest DOM types

brophdawg11 · 2023-06-14T21:16:50Z

🥳 Thanks for all the work that led to this @jenseng!

github-actions · 2023-06-21T16:09:56Z

🤖 Hello there,

We just published version 6.14.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

github-actions · 2023-06-23T19:58:15Z

🤖 Hello there,

We just published version 6.14.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

remix-cla-bot bot added the CLA Signed label Jan 10, 2023

This was referenced Jan 10, 2023

fix(react-router-dom): fix submitter serialization (alt approach) #9866

Closed

fix(remix-react): fix submitter serialization remix-run/remix#4474

Closed

jenseng force-pushed the fix-form-submitters branch from 100e8b8 to 5cfa6b7 Compare January 10, 2023 01:57

MichaelDeBoey added the react-router-dom label Jan 27, 2023

MichaelDeBoey requested changes Jan 27, 2023

View reviewed changes

brophdawg11 self-assigned this Jan 27, 2023

jenseng marked this pull request as draft January 30, 2023 16:23

brophdawg11 removed their assignment Feb 1, 2023

github-actions bot added the needs-response label Apr 17, 2023

github-actions bot removed the needs-response label Apr 18, 2023

jenseng mentioned this pull request May 5, 2023

chore: upgrade jest and jsdom #10453

Merged

jenseng force-pushed the fix-form-submitters branch 3 times, most recently from 2e1f522 to f1d2a11 Compare May 25, 2023 23:08

jenseng requested a review from MichaelDeBoey May 25, 2023 23:10

jenseng marked this pull request as ready for review May 25, 2023 23:10

jenseng force-pushed the fix-form-submitters branch from f1d2a11 to b009d77 Compare May 25, 2023 23:32

jenseng commented May 25, 2023

View reviewed changes

brophdawg11 self-assigned this May 26, 2023

jenseng commented May 26, 2023

View reviewed changes

packages/react-router-dom/__tests__/data-browser-router-legacy-formdata-test.tsx Outdated Show resolved Hide resolved

MichaelDeBoey approved these changes Jun 6, 2023

View reviewed changes

jenseng and others added 5 commits June 14, 2023 09:02

chore: fix comment

317833b

Bump jsdom, change polyfill appoach in tests

c9a473e

inline buildFormData

94ff578

remove @types/web now that we're on the latest TypeScript

ebf92b8

this was previously needed to get the latest DOM types also bump packages sizes

jenseng force-pushed the fix-form-submitters branch from 23db0cd to ebf92b8 Compare June 14, 2023 15:10

Merge branch 'dev' into fix-form-submitters

7e2d339

brophdawg11 approved these changes Jun 14, 2023

View reviewed changes

Update changeset

c297d9b

brophdawg11 merged commit f63774c into remix-run:dev Jun 14, 2023

github-actions bot mentioned this pull request Jun 21, 2023

chore: Update version for release (pre) #10626

Merged

brophdawg11 mentioned this pull request Jun 21, 2023

Add support for json/text submissions remix-run/remix#6570

Merged

jenseng deleted the fix-form-submitters branch June 23, 2023 00:21

github-actions bot mentioned this pull request Jun 23, 2023

chore: Update version for release #10636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jenseng commented Jan 10, 2023 •

edited

Loading

changeset-bot bot commented Jan 10, 2023 •

edited

Loading

jenseng commented Jan 12, 2023

brophdawg11 commented Jan 24, 2023

jenseng commented Jan 24, 2023 •

edited

Loading

brophdawg11 commented Jan 24, 2023

jenseng commented Jan 24, 2023 •

edited

Loading

MichaelDeBoey left a comment

jenseng commented Jan 27, 2023

jenseng commented Jan 30, 2023

brophdawg11 commented Feb 1, 2023

github-actions bot commented Apr 17, 2023

jenseng commented Apr 18, 2023

jenseng commented May 6, 2023

jenseng commented May 25, 2023

jenseng May 25, 2023

brophdawg11 Jun 6, 2023 •

edited

Loading

jenseng Jun 6, 2023

jenseng commented May 26, 2023 •

edited

Loading

jenseng commented May 27, 2023

brophdawg11 commented Jun 6, 2023

jenseng commented Jun 9, 2023

jenseng commented Jun 14, 2023

brophdawg11 commented Jun 14, 2023

github-actions bot commented Jun 21, 2023

github-actions bot commented Jun 23, 2023

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

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

Conversation

jenseng commented Jan 10, 2023 • edited Loading

changeset-bot bot commented Jan 10, 2023 • edited Loading

🦋 Changeset detected

jenseng commented Jan 12, 2023

brophdawg11 commented Jan 24, 2023

jenseng commented Jan 24, 2023 • edited Loading

brophdawg11 commented Jan 24, 2023

jenseng commented Jan 24, 2023 • edited Loading

MichaelDeBoey left a comment

Choose a reason for hiding this comment

jenseng commented Jan 27, 2023

jenseng commented Jan 30, 2023

brophdawg11 commented Feb 1, 2023

github-actions bot commented Apr 17, 2023

jenseng commented Apr 18, 2023

jenseng commented May 6, 2023

jenseng commented May 25, 2023

jenseng May 25, 2023

Choose a reason for hiding this comment

brophdawg11 Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

jenseng Jun 6, 2023

Choose a reason for hiding this comment

jenseng commented May 26, 2023 • edited Loading

jenseng commented May 27, 2023

brophdawg11 commented Jun 6, 2023

jenseng commented Jun 9, 2023

jenseng commented Jun 14, 2023

brophdawg11 commented Jun 14, 2023

github-actions bot commented Jun 21, 2023

github-actions bot commented Jun 23, 2023

jenseng commented Jan 10, 2023 •

edited

Loading

changeset-bot bot commented Jan 10, 2023 •

edited

Loading

jenseng commented Jan 24, 2023 •

edited

Loading

jenseng commented Jan 24, 2023 •

edited

Loading

brophdawg11 Jun 6, 2023 •

edited

Loading

jenseng commented May 26, 2023 •

edited

Loading