[fixed] Prevent unintended register and unregister #808 #1040

honeymaro · 2024-02-13T18:38:54Z

Fixed Cannot register modal instance that's already open #808
Checking this.props.isOpen instead of this.state.isOpen in componentWillUnmount.
Moved the call to this.beforeOpen() from the beginning of the open method to an else block.
Set this.closeTimer without waiting until the end of the setState.
Call this.afterClose() directly after setting the state in closeWithoutTimeout.

Tested environment

React ^18
React.StrictMode / without React.StrictMode
(I tested it with reference to Cannot register modal instance that's already open #808 (comment))

This PR also fixes the problem regarding conditional rendering.

Acceptance Checklist:

Tests
Documentation and examples (if needed)

Fixes #808

- Fixed [reactjs#808](reactjs#808) - Checking `this.props.isOpen` instead of `this.state.isOpen` in `componentWillUnmount`. - Moved the call to `this.beforeOpen()` from the beginning of the `open` method to an else block. - Set `this.closeTimer` without waiting until the end of the setState. - Call `this.afterClose()` directly after setting the state in `closeWithoutTimeout`.

diasbruno · 2024-02-13T21:06:45Z

@honeymaro Looks interesting. Can you test locally with react 16.3+, please? I think 16.3+ should be an ok version to support, and, deprecate the rest.

diasbruno · 2024-02-13T21:08:23Z

And react 17, please.

honeymaro · 2024-02-13T23:47:24Z

@diasbruno Certainly!

honeymaro · 2024-02-14T02:56:20Z

@diasbruno I have tested it on React 16.4 and 17. So far, I found no problem with it.
Here are some videos. Please let me know if my test was wrong.

React 16, Without StrictMode

react16-without-strictmode.mp4

React 16, With StrictMode

react16-with-strictmode.mp4

React 17, Without StrictMode

react17-without-strictmode.mp4

React 17, With StrictMode

react17-with-strictmode.mp4

diasbruno · 2024-02-14T18:18:15Z

Grande lavoro, @honeymaro.

We are going to take some time to stress test this one before we merge, so bear with us.

cc @doeg

doeg · 2024-02-14T21:38:30Z

This is exciting, @honeymaro! 🎉 And thank you for the tag, @diasbruno! I'm happy to take a closer look at this diff over the next couple of days.

For what it's worth, our enormous codebase isn't on React 18 yet but we're hoping to get there in the near future, so this work is very relevant and appreciated. ❤️ In terms of stress testing, I think it's fair to say that we likely have a lot of "interesting" use cases of React Modal so it'll be informative to deploy a test branch with both this change + React 18 and see how it goes. 😹

honeymaro · 2024-02-15T00:24:52Z

Thank you for your kind comments, @diasbruno and @doeg!
I understand the importance of testing, and I also don't want to break this library that's being downloaded 2 million times a week. 🤣
I hope that the changes I have made do not cause any problems. Please do let me know if you come across any issues or concerns. I'll keep testing it out as well!

Thank you again!

doeg · 2024-02-22T14:06:55Z

Thanks all for your patience! 🙏 Just a note to say that I'm testing this out in our codebase now and will report back shortly!

doeg · 2024-02-22T17:47:56Z

tl;dr: This change seems fine on our end. 👍

Below are further notes after testing this change.

First, some initial caveats:

We use ReactModal in many places (easily hundreds) in many "creative" ways, as I mentioned. (We use conditional rendering of React Modal all over the place, for example.)
Whenever we upgrade ReactModal, we tend to set aside a week or more to ensure a safe rollout.
There are several known issues on our end that are preventing us from upgrading to react-modal@3.16.1 (unrelated to this particular change).

So all to say that the testing I did today, though I did my best to be rigorous, is still not as involved as when we upgrade "for real". :)

Second, I can't seem to reproduce the issue described in #808 in our codebase.

This is surprising, as we use conditional rendering (as shown in this simple bug repro here) all over the place, even though conditional rendering is not recommended per this note. :face_with_monocle:

To note what I did:

I installed react@18.2.0 and react-dom@18.2.0 and react-modal@13.6.1 (the latest published version, without this fix)
I opened and closed various modals that use conditional rendering.
- The expected Cannot register modal instance that's already open warning did not appear in the console
- The ReactModal__Body--open was correctly added to body element on modal open and removed on modal close.

For good measure, I added a component that copies the bug repro as-written (including Modal.setAppElement('body'), which we already do) and was still unable to reproduce the warning. 🤔 I'm not yet sure where the discrepancy is.

Third, we're not yet on React 18 and there are several known issues in our codebase that need to be addressed prior to us upgrading.

That caveat aside, I checked out @honeymaro's branch and did dev builds against React 18, React 17, and React 16.

I did notice a few issues but, on further inspection, they happen on react-modal@3.16.1 (i.e., the latest published version without this fix) so... that's more fun stuff for us to fix before we upgrade (😹) but not a blocker for this PR.

Overall, this looks good to me! 🎉

doeg · 2024-02-22T17:50:56Z

src/components/ModalPortal.js

@@ -133,7 +133,7 @@ export default class ModalPortal extends Component {
  }

  componentWillUnmount() {
-    if (this.state.isOpen) {
+    if (this.props.isOpen) {


Perhaps a question for you, @diasbruno, as it's not strictly related to this change -- it looks like the only actual use of this.state.isOpen is this block:

// Focus only needs to be set once when the modal is being opened if ( this.props.shouldFocusAfterRender && this.state.isOpen && !prevState.isOpen ) { this.focusContent(); }

Is that correct? 🤔 Is there a way we could accomplish the programmatic focus of the content without maintaining a this.state.isOpen variable alongside this.props.isOpen? The simplification may be nice, if so.

The state inside the modal is used to manage the timed transition (sync'd with CSS).

When the isOpen flag is changed from true to false, the internal state is true.
react-modal will wait the css transition N milliseconds, then set to false
(same when you are openning).
When the transition has finished, we then do the tear down any other resources
(applied css names to document.body and others).

I always wanted to implement the transition mechanism using css transition events,
but never had a change to see the support for it.

Ahh! Obrigado pela explicação. :)

I always wanted to implement the transition mechanism using css transition events,
but never had a change to see the support for it.

It would be more reliable than setting timers to manage the internal state.

That would be cool! I did see this v4 issue about that: #993

This proposed fix does not work if the modal is closed with a timeout. isOpen will be false when we get here, so afterClose() won't run and the body class will be left on the DOM.

This proposed fix does not work if the modal is closed with a timeout. isOpen will be false when we get here, so afterClose() won't run and the body class will be left on the DOM.

@gillesbouvier-qz Could you please provide me with an example? It would be helpful. Thanks!

Are you foregoing support for this?

isOpen && ( <Modal isOpen={isOpen} closeTimeoutMS={300} onRequestClose={() => { setIsOpen(false); }} ... /> )

If you still want to support the above, I believe isOpen will be false during unmounting and afterClose() won't run leaving the body class in place (and onAfterClose() won't run either). Using the isOpen props seems unsafe to me, as only the modal component should control when it is open or closed.

Maybe I'm missing something?

Wouldn't this solve both problems at once?

Suggested change

if (this.props.isOpen) {

clearTimeout(this.closeTimer);

if (this.props.isOpen || this.closeTimer) {

this.closeWithoutTimeout();

}

It solves the problem for in my testcase:

ModalPortal.js:164 CLOSING MODAL WITH TIMEOUT 300 ModalPortal.js:177 CLOSE WITH TIMEOUT ModalPortal.js:322 UN-MOUNTING ModalPortal.js:184 CLOSE WITHOUT TIMEOUT ModalPortal.js:100 AFTER CLOSE

the idea being that we always want afterClose() to run at least once even when unmounting.

diasbruno · 2024-02-22T18:19:18Z

Just to remind that setState(state, callback) is a legacy way to update state.

Not sure if there are any incompartibilities with the new react state system.

Pointing this out, because some functions have been moved outside
of the setState callback.

Maybe another way to simulate this would be if there is any unhandled exceptions.
So react-modal don't property remove the resources.

doeg mentioned this pull request Feb 16, 2024

Add support for React & Node 18 (as easy as adding startTransition() on each setState) #1019

Open

doeg reviewed Feb 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fixed] Prevent unintended register and unregister #808 #1040

[fixed] Prevent unintended register and unregister #808 #1040

honeymaro commented Feb 13, 2024 •

edited by diasbruno

diasbruno commented Feb 13, 2024

diasbruno commented Feb 13, 2024

honeymaro commented Feb 13, 2024

honeymaro commented Feb 14, 2024 •

edited

diasbruno commented Feb 14, 2024

doeg commented Feb 14, 2024 •

edited

honeymaro commented Feb 15, 2024

doeg commented Feb 22, 2024

doeg commented Feb 22, 2024

doeg Feb 22, 2024

diasbruno Feb 22, 2024 •

edited

doeg Feb 22, 2024

diasbruno Feb 22, 2024

doeg Feb 22, 2024

gillesbouvier-qz Apr 5, 2024 •

edited

honeymaro Apr 5, 2024

gillesbouvier-qz Apr 5, 2024 •

edited

gillesbouvier-qz Apr 5, 2024 •

edited

gillesbouvier-qz Apr 5, 2024 •

edited

diasbruno commented Feb 22, 2024

-    if (this.props.isOpen) {
+    clearTimeout(this.closeTimer);
+    if (this.props.isOpen || this.closeTimer) {
+      this.closeWithoutTimeout();
+    }

[fixed] Prevent unintended register and unregister #808 #1040

Are you sure you want to change the base?

[fixed] Prevent unintended register and unregister #808 #1040

Conversation

honeymaro commented Feb 13, 2024 • edited by diasbruno

diasbruno commented Feb 13, 2024

diasbruno commented Feb 13, 2024

honeymaro commented Feb 13, 2024

honeymaro commented Feb 14, 2024 • edited

diasbruno commented Feb 14, 2024

doeg commented Feb 14, 2024 • edited

honeymaro commented Feb 15, 2024

doeg commented Feb 22, 2024

doeg commented Feb 22, 2024

doeg Feb 22, 2024

Choose a reason for hiding this comment

diasbruno Feb 22, 2024 • edited

Choose a reason for hiding this comment

doeg Feb 22, 2024

Choose a reason for hiding this comment

diasbruno Feb 22, 2024

Choose a reason for hiding this comment

doeg Feb 22, 2024

Choose a reason for hiding this comment

gillesbouvier-qz Apr 5, 2024 • edited

Choose a reason for hiding this comment

honeymaro Apr 5, 2024

Choose a reason for hiding this comment

gillesbouvier-qz Apr 5, 2024 • edited

Choose a reason for hiding this comment

gillesbouvier-qz Apr 5, 2024 • edited

Choose a reason for hiding this comment

gillesbouvier-qz Apr 5, 2024 • edited

Choose a reason for hiding this comment

diasbruno commented Feb 22, 2024

honeymaro commented Feb 13, 2024 •

edited by diasbruno

honeymaro commented Feb 14, 2024 •

edited

doeg commented Feb 14, 2024 •

edited

diasbruno Feb 22, 2024 •

edited

gillesbouvier-qz Apr 5, 2024 •

edited

gillesbouvier-qz Apr 5, 2024 •

edited

gillesbouvier-qz Apr 5, 2024 •

edited

gillesbouvier-qz Apr 5, 2024 •

edited