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

[jquense]

This is a weird one. Basically, if you add an event listener to the document in an effect that was triggered by an event. e.g. click toggles some state, which triggers an effect, which adds a click handler to the document. In the normal case the new event handler will "miss" the triggering event, e.g. the added click handler won't respond to the click event that triggered it being added (omg).

HOWEVER, if you render a portal first, the timing changes slightly and the added event handler will see the current event.

React version: 17

Steps To Reproduce

https://codesandbox.io/s/react-playground-forked-cyt0f?file=/index.js

1. Click the "show Message" button to see a message toggle in and out
2. Click the "Render Portal" button (see a portal rendered into the body)
3. Click the "show Message" button again and notice nothing happens

The reason for the final behavior is the click event both opens and closes the message, (calls set state twice)

Link to code example:

https://codesandbox.io/s/react-playground-forked-cyt0f?file=/index.js

The current behavior

The expected behavior

That they be consistent

[gaearon]

17 only?

[jquense]

17 only! downstream issue https://github.com/react-bootstrap/react-bootstrap/issues/5409

[jquense]

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

[gaearon]

What sounds like a bug here is that useEffect is called synchronously at all. It should normally be deferred — which would avoid this issue. So it sounds like there's an unexpected effect flush. Would be nice to dig into why.

[gaearon]

I'm sooo confused. I can't reproduce this anymore. The only thing I did was to update Chrome...

[gaearon]

Can you still repro it?

[jquense]

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

[gaearon]

Ok at least I can still sleep at night

[gaearon]

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.

[gaearon]

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?

[jquense]

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 asetImmediate would work albeit pretty annoying to manage in an effect.

For anyone looking out 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
})

[eps1lon]

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

[eps1lon]

@gaearon It seems to me that the assertions in https://github.com/facebook/react/blob/c59c3dfe554dafb64864f3bbcfff6ffe51f32275/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js#L1705-L1722 are not correct.

The update that is scheduled in the passive effect is deferred as intended when flushing sync but the effect itself should not run when flushing synchronously.

IMO
```diff
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index 79794bbea7..6ab75eac6d 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -1706,14 +1706,16 @@ describe('ReactHooksWithNoopRenderer', () => {
ReactNoop.flushSync(() => {
_updateCount(2);
});
+

• expect(Scheduler).toHaveYielded(['Count: 2']);
  });

   // As a result we, somewhat surprisingly, commit them in the opposite order.
   // This should be fine because any non-discrete set of work doesn't guarantee order
   // and easily could've happened slightly later too.
   expect(Scheduler).toHaveYielded([

- 'Will set count to 1',
'Count: 2',
+ 'Will set count to 1',
'Count: 1',
]);
```
would make more sense here.

The test comment acknowledges that this is the opposite order and it seems to me that this issue is a good example why this order is problematic: Any side-effect that touches the DOM directly is out-of-order.

[mickaelzhang]

@jquense Not sure about the intended behaviour but it seems like having the following solve your issue (at least temporarely):

document.addEventListener("click", clickHandler, { capture: true })

On step 3, it displays the message correctly

[WesCossick]

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

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