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] Prevent unintended register and unregister #808 #1040

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

honeymaro
Copy link

@honeymaro honeymaro commented Feb 13, 2024

  • 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

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
Copy link
Collaborator

@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
Copy link
Collaborator

And react 17, please.

@honeymaro
Copy link
Author

@diasbruno Certainly!

@honeymaro
Copy link
Author

honeymaro commented Feb 14, 2024

@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
Copy link
Collaborator

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
Copy link

doeg commented Feb 14, 2024

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
Copy link
Author

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
Copy link

doeg commented Feb 22, 2024

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
Copy link

doeg commented Feb 22, 2024

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 [email protected] (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 [email protected] and [email protected] and [email protected] (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 [email protected] (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! 🎉

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

componentWillUnmount() {
if (this.state.isOpen) {
if (this.props.isOpen) {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@diasbruno diasbruno Feb 22, 2024

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Ahh! Obrigado pela explicação. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link

@gillesbouvier-qz gillesbouvier-qz Apr 5, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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!

Copy link

@gillesbouvier-qz gillesbouvier-qz Apr 5, 2024

Choose a reason for hiding this comment

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

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?

Copy link

@gillesbouvier-qz gillesbouvier-qz Apr 5, 2024

Choose a reason for hiding this comment

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

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();
}

Copy link

@gillesbouvier-qz gillesbouvier-qz Apr 5, 2024

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

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.

@TheWirv
Copy link

TheWirv commented May 28, 2024

Hey guys, I was wondering what the current status on this is. We can reproduce this in our codebase with the most recent version v3.16.1, and I'm pretty sure it simply has to do with React's new Strict Mode behaviors of doubling lifecycle function exectution.

Is there any way I can assist in testing this? I tried to install this commit on our side via Git URL dependency, but it just threw "Can't resolve 'react-modal' back at me." Not sure how I can test whether this fix makes the warning disappear in our codebase.

At the end this is not really crucial since the modals work fine, and this warning only appears in dev mode because there React mounts twice in Strict Mode. On the other hand, they do this for a specific purpose, and once they will implement any features where they un-/remount components while keeping state, this package will probably break.

@scvsoft-cruzfernandez
Copy link

Any news on this one? I think my application also needs this.

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.

Cannot register modal instance that's already open
6 participants