Wrap custom events dispatching with flushSync

[Andarist]

fixes #1287

Ok, so this is what I got from a quick debugging session:

1. it's because in React 18 the contextmenuchange isn't dispatched at all in this case and thus you can't prevent it and fire a custom logic for it
2. this in turn is caused by the fact that you set pointer-events: none on the body, here
3. you need to set this to handle some requirements and that is fine, the problem is that in this scenario resetPointerEvents is not called "soon enough" to unblock the mentioned contextmenuchange event
4. In React 17 it actually happens "soon enough" so the whole thing works

I've attached onOpenChange to the ContextMenu.Root and just logged the received value together with some other stuff.

This is the order of the things happening under the hood:
React 17: resetPointerEvents() -> onOpenChange(false)
React 18: onOpenChange(false) -> resetPointerEvents()

As we can see... this somewhat matches the info that I've gathered when debugging. The order is flipped and this leads to a special sort of race condition.

Based on my intuition I've just deduced that this might behave slightly differently in terms of "flushing" the updates as there were some changes to that in React 18. I was looking for a rogue setState, to wrap it with flushSync but then I've realized that during debugging I've seen some CustomEvent stuff and since React is using window.event to determine the update priority... it made sense to just wrap all of those custom events with flushSync. You might want to be more granular, but I think that it's safer to just merge this and then reexamine if some of your custom events should actually not be wrapped with this.

[benoitgrelard]

Hi @Andarist,

Thanks for this. I am not aware of flushSync and have been researching it a bit.
How much do you know about it?
Looking at this here this statement scares me a bit:

You also have an option to flush the entire tree if you know what you’re doing. The API is called ReactDOM.flushSync(fn). I don’t think we have documented it yet, but we definitely will do so at some point during the 16.x release cycle. Note that it actually forces complete re-rendering for updates that happen inside of the call, so you should use it very sparingly. This way it doesn’t break the guarantee of internal consistency between props, state, and refs.

I will look into the original issue a bit more too.

[Andarist]

You can check out the Twitter thread where I've linked to this PR: https://twitter.com/AndaristRake/status/1509537858423566344 This might have some additional interesting info in it. At no point, any React team member has suggested there that this is the wrong solution.

I understand why you might be worried about this here but I would roughly classify this as equivalent to an inferred default priority for all discrete events. It just happens that you are using custom events and thus the builtin detection in React can't classify this automatically as triggered by the user. Note that there might be some subtle differences as this one really flushes the whole update synchronously whereas maybe for discrete events things can be batched in a better way. I think both would flush in the same synchronous frame - but perhaps with flushSync you can end up rerendering multiple times during that synchronous frame (one update would come from within flushSync and another one would have to come from some other place, so the question is also - can another update be scheduled in the same synchronous frame outside of the flushSync "scope"?).

Either way - I think it's worth landing this ASAP and researching the performance implications separately as this should fix bugs for people that are hard to diagnose and I don't think it should come with a severe performance cost, maybe just with a minor one (if at all).

@jjenzz might also have some context for this function as I remember she was asking about it in the React repo quite some time ago

[benoitgrelard]

Thanks for the added details and the link to the tweet.
As you said, it seems the React team agrees that's the way to go.

It might make sense to apply the same change to other places we use custom events (a quick search shows 3 files: DismissableLayer.tsx, Toast.tsx and Tooltip.tsx).

One thing I am unclear about, is whether this only applies to custom events created using the CustomEvent constructor, or also to custom events created using the Event constructor. I'm asking because we use those a fair few also.

I appreciate your help on this 🙏

[benoitgrelard]

We will also need to add react-dom as a peer dependency of any package that now uses it and also to any of the packages that depend on one of those.

[Andarist]

It might make sense to apply the same change to other places we use custom events (a quick search shows 3 files: DismissableLayer.tsx, Toast.tsx and Tooltip.tsx).

Wrapped all of them.

One thing I am unclear about, is whether this only applies to custom events created using the CustomEvent constructor, or also to custom events created using the Event constructor. I'm asking because we use those a fair few also.

I believe that this would apply to Event constructor as well. It's just that React is internally determining the priority of the update based on window.event.type and thus if you use custom event names/types your updates might not be treated as coming from "discrete" events.

We will also need to add react-dom as a peer dependency of any package that now uses it and also to any of the packages that depend on one of those.

I've added the peer dep to dismissable-layer, toast and tooltip already had it. I've also checked all dismissable-layer dependants and all of them had a peer dep on react-dom.

[Andarist]

In here, I've decided to wrap the whole onChange handler to "batch" all updates scheduled by within this callback

[Andarist]

I guess I forgot to update the lock file - just fixed that

[benoitgrelard]

Can you run yarn version check --interactive locally and patch them. May need to run a second time to decline the main workspace after too.

🙏

[Andarist]

@benoitgrelard I've just done this - please verify if this is how you want to bump packages based on this change

[benoitgrelard]

It's all good apart from the main workspace which should be declined instead of patch 👍

[benoitgrelard]

I believe that this would apply to Event constructor as well. It's just that React is internally determining the priority of the update based on window.event.type and thus if you use custom event names/types your updates might not be treated as coming from "discrete" events.

Do you think we should do those too then?

[andy-hook]

Closing in favour of #1378, thanks again for your help on this 🙏