[fixed] Prevent unintended register and unregister #808 #1040
Conversation
- 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`.
|
@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. |
|
And react 17, please. |
|
@diasbruno Certainly! |
|
@diasbruno I have tested it on React 16.4 and 17. So far, I found no problem with it. React 16, Without StrictMode react16-without-strictmode.mp4React 16, With StrictMode react16-with-strictmode.mp4React 17, Without StrictMode react17-without-strictmode.mp4React 17, With StrictMode react17-with-strictmode.mp4 |
|
Grande lavoro, @honeymaro. We are going to take some time to stress test this one before we merge, so bear with us. cc @doeg |
|
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. 😹 |
|
Thank you for your kind comments, @diasbruno and @doeg! Thank you again! |
|
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! |
|
tl;dr: This change seems fine on our end. 👍 Below are further notes after testing this change. First, some initial caveats:
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:
For good measure, I added a component that copies the bug repro as-written (including 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 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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That would be cool! I did see this v4 issue about that: #993
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This proposed fix does not work if the modal is closed with a timeout.
isOpenwill befalsewhen we get here, soafterClose()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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Wouldn't this solve both problems at once?
| if (this.props.isOpen) { | |
| clearTimeout(this.closeTimer); | |
| if (this.props.isOpen || this.closeTimer) { | |
| this.closeWithoutTimeout(); | |
| } |
There was a problem hiding this comment.
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.
|
Just to remind that Not sure if there are any incompartibilities with the new react state system. Pointing this out, because some functions have been moved outside Maybe another way to simulate this would be if there is any unhandled exceptions. |
|
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. |
|
Any news on this one? I think my application also needs this. |
this.props.isOpeninstead ofthis.state.isOpenincomponentWillUnmount.this.beforeOpen()from the beginning of theopenmethod to an else block.this.closeTimerwithout waiting until the end of the setState.this.afterClose()directly after setting the state incloseWithoutTimeout.Tested environment
React^18React.StrictMode/ withoutReact.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:
Fixes #808