React 17 submit event not bubbling

[dreadheadsic]

Because of the changes in way react 17 attaches event listeners, submitting the form programatically no longer works because submit event doesn't bubble.
Solution for this would be to add bubbles: true when dispatching CustomEvent submit.

[epicfaace]

Thanks, would you be able to make a PR to fix this?

[dreadheadsic]

sure, why not, it's a one liner.

[timothyallan]

Applied the PR manually and it fixes a bunch of headaches for us, thanks! Will it be pushed to a release soon in case we forget to update manually again?

[shanemiller89]

Same as above, fix works great, appreciated! Also wondering if this will be pushed to release soon?

[epicfaace]

See @dreadheadsic 's comment (#2105 (comment)), this issue doesn't appear to be a problem with rjsf but rather caused by inadvertently running two different React versions at the same time. Does that fix it for any of you @timothyallan @shanemiller89 ?

[timothyallan]

This is happening to us on a fairly large production SPA. If we were running multiple versions of React, I would hope that we'd be aware of it :)

[shanemiller89]

Same, we are using this on a stand alone SPA. We were only running React v. 17.01

[dreadheadsic]

@timothyallan @shanemiller89 react-jsonschema-form's peerDependency is react 16 so whatever version is your app using, rjsf will use 16, therefore the issue with two versions.
You can test this by cloning this repo, change peerDependency to react 17 and use it inside your app.
Please correct me if i'm wrong. @epicfaace

[shanemiller89]

Yep, changing the peerDependency to react 17 does fix the issue.

We will run a forked version until rjsf is updated.

Thanks for the assist y'all 👍

[epicfaace]

Hmm, I'm a bit confused -- I thought the peerDependency is set to >=16? (see https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/core/package.json#L41-L43)

I would have thought that that would mean that if you already have React 17, it would keep using that, rather than also using React 16.

Any idea how to fix this on rjsf's end without having to changing the minimum React version supported to 17?

[dreadheadsic]

@epicfaace that works only for minor and patch version differences. For major versions you have to explicitly specify it in peerDependencies eg. "react": "^16.x || ^17.x",

[robertedjones]

I have forked the repo and changed the peer dependency to 17 but the issue is not fixed for me. However, adding bubbles: true as suggested by @dreadheadsic does seem to fix it.

[paintedbicycle]

I've created two CodePens, one with React 16 and one with React 17 in order to try to show the effect of this. Use with React 17 is definitely causing submission issues. In my case it was for programatic form submission.

You can see the details and example Pens here: #2255

[timothyallan]

We still have to remember to manually change the rjsf package each time we re-install node_modules, or we cannot do programmatic form submissions.

[epicfaace]

Hmm, ok, so would setting the peer dependency to "react": "^16.x || ^17.x", fix this issue?

[dreadheadsic]

Hmm, ok, so would setting the peer dependency to "react": "^16.x || ^17.x", fix this issue?

Yes, that will absolutely fix the issue.

[dreadheadsic]

We still have to remember to manually change the rjsf package each time we re-install node_modules, or we cannot do programmatic form submissions.

@timothyallan offtopic, there's a great tool for that kind of situations so you don't have to change manually each time - https://github.com/ds300/patch-package

[timothyallan]

We still have to remember to manually change the rjsf package each time we re-install node_modules, or we cannot do programmatic form submissions.

@timothyallan offtopic, there's a great tool for that kind of situations so you don't have to change manually each time - https://github.com/ds300/patch-package

Thanks! We do have some docker scripts which do some automated bandaging of packages, but this seems a bit tidier!

[paintedbicycle]

Anything I can do to unstick this? What's holding us back?

[epicfaace]

Yeah, if you could make a PR that sets the peer dependency, and which also adds a test with React 17 in some way to ensure this doesn't regress, then that would be quick & easy to merge.

[dreadheadsic]

@epicfaace i'm curious, how would you proceed with writing test for this with current testing setup, cause to me it seems like there's no way. The closest thing that comes to my mind is setting up a bug reproduction repo and testing there.

[epicfaace]

Perhaps we could do something like this (https://medium.com/welldone-software/two-ways-to-run-tests-on-different-versions-of-the-same-library-f-e-react-17-react-16-afb7f861d1e9) to test with React 17 -- using npm aliases instead of yarn aliases, and running the "submit event should bubble" test on the core package.

[paintedbicycle]

I've got a PR for discussion here.

1. It updates the peer dependency
2. It suggests an approach for testing React 16 and React 17
3. It adds the ability to test the Form_tests.js file in both React 16 and React 17 by running npm run test and npm run test-react-17

Note: It needs discussion and cleanup before merge

[mucluck]

form.current.onSubmit({ 
    preventDefault: () => Function, 
    persist: () => Function 
})

Try this

[IanSavchenko]

It's a bit sad to see that this problem doesn't get resolved considering solution is a one-liner.

Here's a workaround that seems to work, if anyone is looking:

(formRef.current as any).formElement.dispatchEvent(
  new CustomEvent('submit', {
    cancelable: true,
    bubbles: true, // <-- actual fix
  }),
);

Please, let me know if you see any immediate issues with this.

[timothyallan]

It's a bit sad to see that this problem doesn't get resolved considering solution is a one-liner.

Here's a workaround that seems to work, if anyone is looking:

(formRef.current as any).formElement.dispatchEvent(
  new CustomEvent('submit', {
    cancelable: true,
    bubbles: true, // <-- actual fix
  }),
);

Please, let me know if you see any immediate issues with this.

That's what we've been patching manually for the past 6 months or so and it's been working fine.

[paintedbicycle]

We do have PRs for this, but we're trying to sort out an issue with setting up testing around that fix. Testing is proving harder than the fix. That's the TL;DR on what's going on. If you want to see more/help, here's the info:

I forked and created a branch that applied the above peerDependency fix. Then I added a test for programatic form submission using the example from the docs. @epicfaace pulled that PR into this repo in order to run the suite of tests set up in Github. You can see that here: #2351

You can also see how he set up the new testing environment (both React 16 and React 17) here: #2332 (comment)

The issue is that the tests passed without the fix applied in both React 16 and 17. You can see that in the commit history - we reverted and re-applied the fix and let the tests run. They should have failed in React 17 without the fix and then worked once the fix was applied.

If we can sort out why that happened and then therefore be more confident about the testing and the fix, it should be easier to merge this fix.

If anyone wants to chat, we're also testing out a Discord server for rjsf here: https://discord.gg/SPXkvQrS

[epicfaace]

@IanSavchenko @timothyallan @paintedbicycle

Basically, about this issue, I'm hesitant about merging a fix until we figure out:

• what exactly is the underlying issue?
• why would the fact that React 17 no longer attaches event handlers at the document level prevent you from submitting the form programmatically?
• what is the best fix that fixes the underlying issue -- bubbles: true or changing the peerDependency?
• how can we add a test that ensures this doesn't regress?

I'm sure this could be figured out with a couple hours of work / investigation, but it's not a priority for me to do all that work for free now ☹️

If someone is able to spend some time and get to the bottom of these questions, then we'd be in a much better place to merge it.

[dreadheadsic]

Yup, i Agree with @epicfaace 100%, it is still not clear enough what is causing this issue and what would the best solution be,
so i did some more testing/research and am more certain that changing the CustomEvent is correct fix.

Npm v7 now has a --legacy-peer-deps option for install which ignores older peer deps. So i ran that and went through node_modules to assert only React 17 is installed. After confirming i went and started the app, and the submit event wasn't being triggered!

Then i've found this issue which pretty much confirms that.
Especially this comment which suggests to use form.requestSubmit instead of letting the "submit" event bubble.

[epicfaace]

@dreadheadsic yep that makes sense. It looks like we added the following line with Event in #1058:

this.formElement.dispatchEvent(new Event("submit"));

Then, we changed it to CustomEvent in #1432:

this.formElement.dispatchEvent(new CustomEvent("submit", { cancelable: true }));

I'm not sure why we called dispatchEvent on a new Event object in #1058. It seems like the solution here would be just to call form.requestSubmit instead?

If we agree (and check) that form.requestSubmit() is the right fix, I think this satisfies my first three questions, honestly, because creating a CustomEvent seems to be a hacky solution anyway, and form.requestSubmit() seems better.

We should figure out the final question though -- how can we add a test that ensures this doesn't regress?

[josh08h]

@paintedbicycle Do you know if this issue is still being worked on? Would love to get the fix in for this one.

[paintedbicycle]

I've lost track of the status of it. There were several approaches to fixes in different PRs, but I don't remember if there was ever one PR with both the fix and passing tests on React 16 and 17 in one spot

[kfcaio]

Is it fixed?

[epicfaace]

Not yet.

if there was ever one PR with both the fix and passing tests on React 16 and 17 in one spot

This is exactly what we need!