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

React 18 different behavior of ReactDom.createRoot and ReactDom.render #23097

Closed
mkinfrared opened this issue Jan 12, 2022 · 2 comments
Closed
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion

Comments

@mkinfrared
Copy link

mkinfrared commented Jan 12, 2022

React 18.0.0-rc.0

I noticed a different behavior when using new ReactDom.createRoot feature comapared to the current ReactDom.render

So I have this demo project here to illustrate the problem.

The setup is pretty simple. The app component has a boolean state to open/close modal. The modal component calls a custom hook useOutsideClick whick adds a click event listener to the window and calls a callback passed to that hook if the click was triggered outside the ref element which is passed as arguments to that hook.

When I use the current ReactDom.render everything works fine as expected. You click the button, the modal opens, the window click event is added, then you click outside the ref object, the modal is closed. But when I switch to new ReactDom.createRoot which is reccomended when using React 18, I get a different behavior. When I click the button to open a modal, the addEventListener inside useEffect in useOutsideClick is called during the bubbling phase, the modal is mounted, then the click event bubbles up to the window which now has a click event registered, and as the click happened on the button which is outside ref object the modal is closed.

I am not really sure if that new behavior is a bug or a feature but I at least would like to hear some explanation on that.

In indes.tsx file I left the code for createRoot commented out so you could easily switch between these two apis.

@mkinfrared mkinfrared added React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion labels Jan 12, 2022
@bvaughn
Copy link
Contributor

bvaughn commented Jan 12, 2022

The repro above could be simplified a lot, but it's demonstrating a similar pattern as I remember accounting for in DevTools some time ago (#21173). This is the solution I went with at the time:

// Delay until after the current call stack is empty,
// in case this effect is being run while an event is currently bubbling.
// In that case, we don't want to listen to the pre-existing event.
let timeoutID = setTimeout(() => {
timeoutID = null;
// It's important to listen to the ownerDocument to support the browser extension.
// Here we use portals to render individual tabs (e.g. Profiler),
// and the root document might belong to a different window.
ownerDocument = ((modalRef.current: any): HTMLDivElement).ownerDocument;
ownerDocument.addEventListener('keydown', handleDocumentKeyDown);
if (dismissOnClickOutside) {
ownerDocument.addEventListener('click', handleDocumentClick, true);
}
}, 0);

Although I also wrote about a few alternative solutions here, if you are interested:
https://gist.github.com/bvaughn/fc1c3f27f1aab91c7378f2264f7c3aa1

The main issue at hand here is that React synchronously runs effects (including passive effects) inside of events like "click" events. (Note that this behavior was always possible, if a layout effect scheduled a state update– which had to be processed synchronously– then passive effects would be flushed in between.) The decision was made by the React team to reduce the complexity of imperative code (in effects) by making their behavior more consistent and predictable. I think this makes things simpler in the common case, although it can lead to some awkward edge cases like the one we're discussing.

cc @gaearon @sebmarkbage in case you're aware of additional context or if I'm misremember anything.

@mkinfrared
Copy link
Author

mkinfrared commented Jan 13, 2022

Thank you @bvaughn for a quick reply and your examples. I came up with a solution of wrapping it inside setTimeout but was not quite sure if that was a correct way of solving the problem. But the trick with performance looks really neat. I should definetly add it to my tool belt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion
Projects
None yet
Development

No branches or pull requests

2 participants