Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(react-router-dom): streamline jsdom submitter bug workaround #9824

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

jenseng
Copy link
Contributor

@jenseng jenseng commented Jan 6, 2023

Work around the submitter bug in just one place, and link to my jsdom PR which will fix it, so that the workaround can be removed sooner rather than later 🤞

This workaround refactor also establishes a pattern for other jsdom bug polyfills which will be landing in forthcoming RR PRs (the bugs aren't relevant in the current test suite, but will be in the PRs 😅)

@changeset-bot
Copy link

changeset-bot bot commented Jan 6, 2023

🦋 Changeset detected

Latest commit: 168e557

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 6, 2023

Hi @jenseng,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 6, 2023

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@jenseng
Copy link
Contributor Author

jenseng commented Jan 6, 2023

cc @brophdawg11

Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor questions but this looks great - thanks!

@@ -37,6 +37,9 @@ import {
useLocation,
createRoutesFromElements,
} from "react-router-dom";
import { isSubmitterElement } from "../dom";

fixSubmitEventSubmitter(window);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were to put this in another file, would we dup the listeners on window? Wondering if it should either return an unlisten and be called in a beforeEach/afterEach setup? Or - could this just move to setup.ts and be applied globally for all react-router-dom tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell of how jsdom/rtl are wired up, it looks like we just need to do it once per test suite/file, rather than per test. And since jest runs each test file in an isolated worker, I don't believe any cleanup should be needed. I like the setup.ts idea, I'll move it there 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 4224 to 4225
if (maybeSubmitter?.form === event.target)
event.submitter = maybeSubmitter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth making this polyfill-esque so it becomes a no-op once this is fixed upstream?

      if (maybeSubmitter?.form === event.target && !event.hasOwnProperty('submitter'))
        event.submitter = maybeSubmitter;

Or maybe a more lax version that would still overwrite undefined/null values:

      if (maybeSubmitter?.form === event.target)
        event.submitter = event.submitter || maybeSubmitter;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I like the polyfill idea, I'll tweak this a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Work around the submitter bug in just one place, and link to my jsdom PR
which will fix it, so that the workaround can be removed sooner rather
than later 🤞

This workaround refactor also establishes a pattern for other jsdom bug
polyfills which will be landing in forthcoming RR PRs (the bugs aren't
relevant in the current test suite, but will be in the PRs 😅)
@brophdawg11 brophdawg11 merged commit c50b5ac into remix-run:dev Jan 9, 2023
@brophdawg11
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants