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 react-bootstrap/react-bootstrap#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 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
})

[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 7f28234...4c8c98a

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 #20074 (comment)

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

react/packages/react-reconciler/src/ReactFiberWorkLoop.old.js

Lines 1061 to 1064 in 3fbd47b

	// TODO: Should be able to flush inside batchedUpdates, but not inside `act`.
	// However, `act` uses `batchedUpdates`, so there's no way to distinguish
	// those two cases. Need to fix this before exposing flushDiscreteUpdates
	// as a public API.

. I'm generally suprised that passive effects are flushed synchronously at all but this does seem to be intended

react/packages/react-reconciler/src/ReactFiberWorkLoop.old.js

Lines 1083 to 1085 in 3fbd47b

	// If the discrete updates scheduled passive effects, flush them now so that
	// they fire before the next serial event.
	flushPassiveEffects();

[eps1lon]

@gaearon It seems to me that the assertions in

react/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

Lines 1705 to 1722 in c59c3df

	act(() => {
	ReactNoop.flushSync(() => {
	_updateCount(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',
	'Count: 1',
	]);
	expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
	});

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 --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);

[BrinsterAman]

Is there a fix in progress, for upcoming versions, for this bug? I am facing this issue mui/material-ui#24783 , which cant be resolved because of this.

[eps1lon]

I am facing this issue mui/material-ui#24783 , which cant be resolved because of this.

Could you explain in the Material-UI issue why this can't be resolved for you? We were under the impression that we worked around this bug for our specific usage.

For updates on a fix, #20074 (comment) is the latest update. From what I gather the team is already experimenting with potential fixes. You can subscribe to this (#20074) github issue to get notified about updates. Asking for updates does not speed up delivery of said update.

[gaearon]

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: #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.

[Kianibound]

Bug; Still clickAwayListener tests fails using React testing library. It seems because of react18.