Modify useEditorView to flushSync all dispatched transactions

[ilyaGurevich]

This change ensures a flushSync-only behavior when dispatching transactions. Because react 18 automatically batches state updates, we can also remove references in the code where we used unstable_batchedUpdates in general.

[smoores-dev]

@ilyaGurevich I'm not sure the ref is getting us what we want here! Is it possible that we just want to useMemo the call to withBatchedUpdates? It seems like the new layout effect hook is updating the ref any time the dependencies change, which should be the same as how useMemo works.

[smoores-dev]

Also, how would you feel about trying to add a unit test to capture this use case? Feels edge-case-y enough to earn one!

[tilgovi]

I'm confused and want to investigate a little bit more. Using flushSync actually creates a batch boundary. We do it so that if dispatchTransaction triggers a React state change that React doesn't flush any effects of that state change until we unwind the stack. We introduced this to prevent nested Redux dispatches when components have effects that dispatch transactions (like something dispatching on mount).

So, using flushSync prevents this dispatch from getting rolled up into a larger batch, to flush all together, but it doesn't avoid batching. I'd be shocked if the IME composition case is part of a larger batch that isn't otherwise flushed synchronously. At least in React 17, event handling always happened within a batch and that batch flushed synchronously.

[tilgovi]

I found one bit of interesting info here. The actual dispatch in the case of typing is coming from ProseMirror reading a DOM mutation, not from an event. On React 17, that would be unbatched. On React 18, that'd be batched but not synchronous.

I have suggested previously that we should always flush sync, since editor input is high priority for proper user experience. The only reason I hesitated was that I was afraid we had code at NYT that had components dispatching on mount, which is not a legal place to do a flushSync.

I'd be happy to synchronously flush unconditionally here, and work out issues with dispatching at bad lifecycle moments downstream.

[tilgovi]

In other words, because ProseMirror uses MutationObserver rather than input events, letting the browser handle most editing operations, resulting React state changes have always been unbatched, which means they've also committed synchronously. We added the batching to ensure that we didn't cause nested dispatches when components dispatch in lifecycle hooks. Going a step further, we could commit that batch synchronously, which would be ideal for an editor.

I think this is on the right track, but doesn't go far enough, and the justification isn't quite correct. We don't want to opt out of batching. We want to be sure the batch commits synchronously.

[ilyaGurevich]

I'd be happy to synchronously flush unconditionally here, and work out issues with dispatching at bad lifecycle moments downstream.

I'm ok doing this as well, I think it might also make this overall logic simpler to reason around without needing to specifically understand what composition is in general. What sorts of examples of bad lifecycles moments would you expect in this case? Like a bad case of useEditorEffect?

I think I'm also still somewhat confused on why:

• we were purposefully batching dispatch transactions in react 17
• flushSync'ing these transactions is acceptable now in react 18

[tilgovi]

Like a bad case of useEditorEffect?

Yeah. It's risky to dispatch transactions in useEditorEffect because it can lean to infinite dispatch loops. Using correct dependencies or conditionals can avoid that. I was afraid we would have cases where it is correct, though, which is why I hesitated to use flushSync() originally. Batching updates was the gentler version of flushSync().

I think I'm also still somewhat confused on why:

• we were purposefully batching dispatch transactions in react 17
• flushSync'ing these transactions is acceptable now in react 18

We were purposefully batching transactions so that when a dispatch causes a state change (generally via Redux), it doesn't cause React to invoke any component lifecycle hooks until after we return from the call to dispatchTransaction. That way we avoid nested dispatches.

Using flushSync() would have been acceptable on React 17, too, I just wasn't sure it was acceptable from a developer ergonomics perspective because it'll throw warnings in the console if you dispatch from a component lifecycle update.

[tilgovi]

Just to reiterate/clarify: flushSync() == commit(batch())`. It is stronger.

[ilyaGurevich]

Yeah. It's risky to dispatch transactions in useEditorEffect because it can lean to infinite dispatch loops

I'd be curious about making a custom eslint configuration here to provide warnings when you do stuff like this, similar to calling setState in a useEffect which is discouraged sometimes

[tilgovi]

I'd be curious about making a custom eslint configuration here to provide warnings when you do stuff like this, similar to calling setState in a useEffect which is discouraged sometimes

Yeah, it'd be nice, but I think it's low on the priorities for this repo. Right now, we're not even sure we have the implementation at all how we'd like it.

[smoores-dev]

Thanks for this, @ilyaGurevich! Would you mind rebasing on top of the latest on main so that the unit tests get run on this branch? Never mind it seems that's already happening!

[smoores-dev]

@ilyaGurevich do you need a new npm release for this?

[ilyaGurevich]

@ilyaGurevich do you need a new npm release for this?

Not yet, I have a follow-up PR I'd like to open and discuss first before doing npm release!