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

Fixed error where crash will happen if fired twice in a row. #108

Merged
merged 3 commits into from
Feb 20, 2020

Conversation

robhybrid
Copy link

No description provided.

Copy link
Member

@limonte limonte left a comment

Choose a reason for hiding this comment

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

Thanks @robhybrid for the PR!

  1. Could you please be more specific about "crashing"? I guess there should be an exception(s) thrown? If so, please provide the code to reproduce the issue.
  2. You've added the test (which is great!) but it's passing for the current master without the if (domElement) { statement. So the test isn't really covering the issue you're trying to fix. Ideally, the test should fail without changes in the actual code.

@robhybrid
Copy link
Author

robhybrid commented Feb 20, 2020

I am getting the following error:

    unmountComponentAtNode(...): Target container is not a DOM element.

      45 |           params.onDestroy = (element) => {
      46 |             superOnDestroy(element)
    > 47 |             ReactDOM.unmountComponentAtNode(domElement)
         |                      ^
      48 |           }
      49 |         }
      50 |       })

      at Object.unmountComponentAtNode (node_modules/react-dom/cjs/react-dom.development.js:27628:13)
      at Object.params.onDestroy (src/index.js:47:22)

This happens with my test, and in my production application.

@robhybrid
Copy link
Author

The test fails on the CI when my change is removed and passes when it is added back. There may be something out of sync with your local environment.

Copy link
Member

@limonte limonte left a comment

Choose a reason for hiding this comment

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

The test fails on the CI when my change is removed and passes when it is added back. There may be something out of sync with your local environment.

It was out of sync indeed, my apologies! Thanks a lot for the well-done contribution!

@limonte limonte merged commit c592b0c into sweetalert2:master Feb 20, 2020
limonte pushed a commit that referenced this pull request Feb 20, 2020
## [3.0.1](v3.0.0...v3.0.1) (2020-02-20)

### Bug Fixes

* unmountComponentAtNode(...): Target container is not a DOM elementd fyf ([#108](#108)) ([c592b0c](c592b0c))
@limonte
Copy link
Member

limonte commented Feb 20, 2020

🎉 This PR is included in version 3.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

limonte pushed a commit that referenced this pull request Nov 6, 2022
…ntd fyf (#108)

* Fixed error where crash will happen if fired twice in a row.

* undid change to see if test fails in CI.

* re-fixed error:  unmountComponentAtNode(...): Target container is not a DOM element.
limonte pushed a commit that referenced this pull request Nov 6, 2022
## [3.0.1](v3.0.0...v3.0.1) (2020-02-20)

### Bug Fixes

* unmountComponentAtNode(...): Target container is not a DOM elementd fyf ([#108](#108)) ([2d885a6](2d885a6))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants