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

[jenseng]

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]

🦋 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]

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]

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]

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]

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]

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]

The polyfills are no longer needed when using jsdom@21.1.0, but jest still requires jsdom@20.x ... I'll see if I can move the needle there

[jenseng]

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]

@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]

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]

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]

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]

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]

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

[brophdawg11]

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

[jenseng]

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

[jenseng]

Two slight variations we could consider:

1. 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.
2. 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]

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

[brophdawg11]

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]

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

[jenseng]

rebased and removed @types/web since typescript@5.1 gives us the latest DOM types

[brophdawg11]

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

[github-actions]

🤖 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]

🤖 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!