react: Bug: useEffect Timing changes depending on if Portal was rendered

What happens here is that the native event bubbles:

• On the root container
• On the body (portal target)

This is why we get into dispatchEvent twice. Root container pass schedules an effect, and body pass flushes it.

Seems like a bug that we flush the effect in this case.

The team has taken a closer look at this now that the related refactors are landing.

We can’t fix it in a general case without relying on some heuristic. Making all effects synchronous would be bad. Making all of them asynchronous also doesn’t work because sometimes we need to flush earlier. The need to flush earlier relates to handling discrete events (like click) that need to be able to observe the already flushed state.

A reasonable compromise is to make them synchronous at the end of discrete events (such as click), but asynchronous in other cases. This mirrors the recent changes we’ve made to state updates in Concurrent Mode to make it easier to adopt. So we added this heuristic in Concurrent Mode: https://github.com/facebook/react/pull/21150. I checked that this issue doesn’t repro in Concurrent Mode with the latest master commit and now consistently does not show the message (because clicks lead to synchronous effects).

We’ve considered backporting this behavior to React 17. But at this point it’s very risky because it’s such a subtle change. It’s also risky because it would be the first time that we rely on window.event.type-based heuristics outside of Concurrent Mode. So it’s kind of a new territory. Given that it’s easy to work around, it doesn’t seem like this risk is warranted in React 17.

So we’ve concluded that, while unfortunate, it is safer to not try to fix it in React 17. But Concurrent Mode will have a more predictable behavior here. (Note that it’s also quite likely that React 16 also had cases with over-flushing, but triggered under different conditions.) I know this isn’t the resolution that most people were hoping for.

well tbh maybe this was present but not observable since before 17 you couldn’t attach an event listener “higher up” so there wasn’t any way to get in front of the current event

You can also fix the issue by wrapping document.addEventListener() in a setTimeout(), like so:

setTimeout(() => {
    document.addEventListener('click', clickHandler);
}, 1);

What I noticed is that for an act(() => trigger.click()) everything works fine but an actual user click causes the issue described: https://codesandbox.io/s/material-ui-issue-forked-gjmkc?file=/src/index.js

Edit: I think I’m try attaching a capture listener as well and then check in the bubble event listener if I also captured the event. That fixes the problem at least for mounts in the bubble phase. That looks promising so far: https://codesandbox.io/s/material-ui-issue-forked-683b2?file=/src/index.js

Edit2: Bisecting releases leads to good: 0.0.0-experimental-7f28234f8 bad: 0.0.0-experimental-4c8c98ab9 I used experimental releases since the @next release channel was released less often in that time. So the bug got introduced in https://github.com/facebook/react/compare/7f28234f8...4c8c98ab9

Edit3: I don’t think the timing of effects changed between 16 and 17. Looks like the timing of events changed due to the changes to the event delegation system. If you add the native listener to document.body instead of document, the code works fine.

Edit4: ~When the portal is rendered into document.body react attaches its event delegator onto the document.body. If I remove that event handler the message can be displayed again.~ See https://github.com/facebook/react/issues/20074#issuecomment-714158332

Edit5: Last bit: What makes this bug hard to track down with tests is that you naturally use act() but within act() React does not flush discrete updates. Though this is already documented in https://github.com/facebook/react/blob/3fbd47b86285b6b7bdeab66d29c85951a84d4525/packages/react-reconciler/src/ReactFiberWorkLoop.old.js#L1061-L1064. I’m generally suprised that passive effects are flushed synchronously at all but this does seem to be intended https://github.com/facebook/react/blob/3fbd47b86285b6b7bdeab66d29c85951a84d4525/packages/react-reconciler/src/ReactFiberWorkLoop.old.js#L1083-L1085

We’ve got a workaround that’s simple and effective in theory here. Need to let folks test it out a bit though. Alternatively I think adding listeners in a setImmediate would work albeit pretty annoying to manage in an effect.

For anyone looking, our current workaround is

useEffect(() => {
  // Note this depends on a deprecated but extremely well 
  // supported quirk of the web platform, this may be less effective 
  // for other event types, YMMV
  const currentEvent = window.event

  element.addEventListener('click', (e) => {
     if (e === currentEvent) {
        currentEvent = null;
        return
     } 
     // do the work
  })

  // ...clean up
})

To follow up on this — it does seem like a bug but we can’t commit to prioritizing it at the moment. There will be a bunch of work done to clean up and simplify related parts of the code in the coming months so we’ll likely address it then. Have you found any userland workaround, or is this a hard blocker for you?

bah sorry @gaearon i was goofing around trying to find a workaround and didn’t realize it was saving instead of forking. It should be back to being reproducible