Bug: createPortal anywhere in the tree makes native events be ran too late

[kosciolek]

Summary

Native events added via useEffect are called too late (and with improper (new) state, rather than the state during listener attachment) if there's a ReactDOM.createPortal anywhere in the tree. First the effect is re-run and a new listener is attached, and only then the native event is called.

React version:
17.0.2

Link to code example:

CodeSandbox

Reproduction

• Anywhere in the tree is a createPortal. It can even be createPortal(null, document.body)
• Somewhere, there is a component with useState.
• It renders a div. The div has an onClick handler passed.
• On top of that, useEffect is used to attach a click event to window
• Both event handlers (both the native event attached to window, and the React event passed to div) use setState.

The current behavior

• The React handler is called. It calls setState
• The component rerenders with the new state, and its effects are re-run.
• Since the effect is re-run, a new native listener is attached (which is bound to the new state)
• Only then, the new listener (with the NEW state) is called.

The expected behavior

• The React handler is called. It calls setState
• The component might rerender or not (doesn't matter)
• The native handler is called with the old state.

In other words

const [popupOpen, setPopupOpen] = useState(false);
useEffect(() => {
   // close popup if clicked anywhere on the screen, some code ignored for brevity 
  const listener = () => {
    console.log('NATIVE', popupOpen);
    if (popupOpen) setPopupOpen(false);
  };
  window.addEventListener("click", listener);
  return () => window.removeEventListener("click", listener);
}, [popupOpen]);

return (
  <>
    <div
      onClick={() => {
        console.log('REACT', popupOpen); // REACT HANDLER STATE
        if (!popupOpen) setPopupOpen(true);
      }}
    >
      Open popup
    </div>
    {ReactDOM.createPortal(null, document.body)}
  </>
);

Logs

But without createPortal, the following is logged:

[kosciolek]

Turns out, strictly a portal is not needed.

Adding this to the described component causes the same bug (?)

const [state, setState] = useState<HTMLDivElement | null>(null);
(...)
<div ref={(ref) => setState(ref)}></div>

[kosciolek]

However, changing it to <div ref={setState} > </div> makes it run just fine, with both the React handler and the native handler showing the same values.

Complete component code:

function App() {
  const [popupOpen, setPopupOpen] = useState(false);
  useEffect(() => {
    console.log('binding', popupOpen);
    const listener = () => {
      console.log("NATIVE", popupOpen);
      if (popupOpen) setPopupOpen(false);
    };
    window.addEventListener("click", listener);
    return () => window.removeEventListener("click", listener);
  }, [popupOpen]);

  const [state, setState] = useState<HTMLDivElement | null>(null);

  return (
    <>
      <div
        onClick={() => {
          console.log("REACT", popupOpen); // REACT HANDLER STATE
          if (!popupOpen) setPopupOpen(true);
        }}
      >
        Open popup
      </div>
      <div ref={setState}></div>
    </>
  );
}

[eps1lon]

Thanks for the report.

Could you check if this is similar to #20074?

[kosciolek]

Yes, it's the same. If portal is rendered, first the event is re-attached, and only then fired.

Though, as above, it's not exclusive to portals. I'd wager anything that triggers another render pass will do that.

First, native events, unless captured, are fired after React's events. Second, native event listeners (as far as my understanding goes) seem to be simply stored in an array. When the event is fired, the browser iterates over that array and fires them one by one. Therefore, an early handler can remove a handler that's after it, and attach a new one. The new one will be fired as well (for the same event), since the browser is just doing a dumb iteration over the array - nothing says we can't modify the array under its feet.

Let's look at this, a less convoluted example:

function App() {
  const [popupOpen, setPopupOpen] = useState(false);

  console.log('RENDER PHASE');

  useEffect(() => {
    console.log("EFFECT (EVENT ATTACHMENT)", popupOpen);
    const listener = () => {
      console.log("NATIVE HANDLER", popupOpen);
    };
    window.addEventListener("click", listener);
    return () => {
      console.log("EFFECT CLEANUP", popupOpen);
      window.removeEventListener("click", listener);
    };
  }, [popupOpen]);

  const [state, setState] = useState<HTMLDivElement | null>(null);

  return (
    <>
      <div
        onClick={() => {
          console.log("REACT HANDLER", popupOpen);
          setPopupOpen((prev) => !prev);
        }}
      >
        Open popup
      </div>
    </>
  );
}

This is fine:

• React handler fired
• Rerender (no effects fired)
• Native handler fire
• Effects fired.

Now, let us add EITHER

1. a portal after the div {createPortal(null, document.body)}
2. A div with a setState for its ref

const [state, setState] = useState<HTMLDivElement | null>(null);
//code
<div ref={(ref) => setState(ref)} />

It's important that it's ref={(ref) => setState(ref)} and not ref={setState} (I don't really get why, the difference is that in the first case, the component rerenders, then rerenders again because of setState(ref), an infinite loop is NOT triggered because ref identity is kept, and setState does not rerender if we re-set the same state. In the second case, a third rerender attempt is not made, because the ref func identity is stable).

Code:

function App() {
  const [popupOpen, setPopupOpen] = useState(false);

  console.log('RENDER PHASE');

  useEffect(() => {
    console.log("EFFECT (EVENT ATTACHMENT)", popupOpen);
    const listener = () => {
      console.log("NATIVE HANDLER", popupOpen);
    };
    window.addEventListener("click", listener);
    return () => {
      console.log("EFFECT CLEANUP", popupOpen);
      window.removeEventListener("click", listener);
    };
  }, [popupOpen]);

  const [state, setState] = useState<HTMLDivElement | null>(null);

  return (
    <>
      <div
        onClick={() => {
          console.log("REACT HANDLER", popupOpen);
          setPopupOpen((prev) => !prev);
        }}
      >
        Open popup
      </div>
      
      {/* Either is enough */}
      <div ref={(ref) => setState(ref)}/>
      {createPortal(null, document.body)}
    </>
  );
}

In this case, this is what happens:

• The React handler is fired.
• Rerender
• Effects are fired! Since the dependency (popupOpen) is different, the clean up is run - old listener, with old state, is removed, a new listener, with new state, is added - The array that holds events is modified mid-flight
• (The browser keeps iterating over the array...)
• Native handler: finally, at the end of the array, it encounters our new native handler. It fires it.

Notice that the effect is fired before the native handler.
This is what is wrong: for some reason, in this configuration, effects are fired before native listeners, and thus they modify the handler array mid-flight.

[eps1lon]

Yes, it's the same.

Closing then as per #20074 (comment)