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

#3711 - @react-refresh removes Ketcher from DOM #3887

Closed
wants to merge 5 commits into from

Conversation

andy197
Copy link

@andy197 andy197 commented Jan 14, 2024

How the feature works? / How did you fix the issue?

The root cause was the setTimeout. Due to the double invoke in Strict Mode, it is possible for the cleanup from the first invocation to run after the second useEffect has already started. This means that before setTimeout get's executed from the first useEffect, there is a new appRoot from the second useEffect, and it get's unmounted when setTimeout function actually executes.

The solution is to check if appRoot has been mounted or not. This will prevent unmounting twice.

appRoot.unmount(); should be synchronous so that it executes immediately after the unmount. I think it's worth it even if we get the
// setTimeout is used to disable the warn msg from react "Attempted to synchronously unmount a root while React was already rendering"
. Otherwise folks who use ketcher with strictmode would not be able to use ketcher at all.

closes #3711

Check list

  • unit-tests written
  • e2e-tests written
  • documentation updated
  • PR name follows the pattern #1234 – issue name
  • branch name doesn't contain '#'
  • PR is linked with the issue
  • base branch (master or release/xx) is correct
  • task status changed to "Code review"
  • reviewers are notified about the pull request

@andy197 andy197 marked this pull request as ready for review January 14, 2024 03:00
@andy197
Copy link
Author

andy197 commented Feb 26, 2024

@StarlaStarla Thank you Starla for the review! What would the next step be for merging this in? 🙂

@andy197
Copy link
Author

andy197 commented Mar 12, 2024

Hi @StarlaStarla!

Sorry for the ping, but just in case you missed this! Is there else that needs to be done?
We would like to get this in so we can use the latest ketcher!

@emripka
Copy link

emripka commented Apr 30, 2024

Hi all! When can we expect to get this PR merged in?

@rrodionov91
Copy link
Collaborator

Hi @emripka @andy197,
We fixed errors in strict mode in this PR #5013

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

Successfully merging this pull request may close these issues.

@react-refresh removes Ketcher from DOM
4 participants