The Future of @nytimes/react-prosemirror

1

Hey all!

A little while ago, we shared our React ProseMirror integration library, @nytimes/react-prosemirror. Thank you so much to everyone who gave feedback, tried out the library, and posted issues on our repo!

For the past several months, I’ve been hard at work on a very different approach to integrate React and ProseMirror. The pull request is here for anyone who wishes to take a look.

The summary is this: the new approach completely replaces ProseMirror’s DOM management system with one built in React. We’re still using prosemirror-view for everything outside of change detection and DOM updates, which means we are exposing exactly the same API. In fact, I’ve ported over most of the unit tests from prosemirror-view to ensure that behavior matches the default library.

This change allows us to have a much cleaner integration with React, especially with regard to implementing node views with React components. Context flows as expected from parents to children, events bubble as expected, and node view components are just React components, without any special work needed. It also fixes a few edge case-y bugs, including significant issues with IME composition input. Overall, I feel confident that this is a huge improvement over the previous implementation, and one that could even be eventually pulled into higher level libraries like ReMirror and TipTap.

This new approach is implemented by subclassing EditorView and overriding some of the default behavior to essentially disable the DOM management. To do this subclassing, we needed to make two very small changes to the EditorView class, and I’m curious about how open you would be, Marijn, to incorporating these changes into prosemirror-view.

  1. We changed updateStateInner to be protected, rather than private. This allowed us to override its implementation in our subclass, to essentially drop everything except for the selection syncing.
  2. We moved all of the side effect-y code out of the EditorView constructor and into a new, protected, init method. This is still called in the constructor, but having it in an override-able method allows us to again drop the pieces of this that are managed by our new, React-based system.

Here’s the place in the pull request where this happens, in case anyone wants to take a look:

There are also a few internal functions that we’re using. It would obviously make my life easier if these were exposed in some way, but I don’t want to ask for a huge increase in the API surface of prosemirror-view if that’s not something you’re interested in. These are the internal functions we’re using:

  • storeScrollPos and resetScrollPos (These are used in our updateStateInner override)
  • selectionFromDOM and selectionToDOM
  • computeDocDeco

Anyway, I’m looking for thoughts and feedback! In particular, Marijn, I’m very interested in hearing whether you would be open to a pull request against prosemirror-view with some or all of these changes. Please let me know if you have any questions!

3 Likes
2

This is interesting.

Your PR mentions replacing the DOM mutation observer with beforeinput handlers. Do I understand correctly that you’re no longer parsing the modified DOM? How do you reliably derive the appropriate changes from these events (which, on platforms like Android Chrome, tend to be fired in weird, incoherent batches)?

What do you use to replace the DOM-linked ViewDesc tree? Or are you still keeping a tree like that and adding it to a DOM node expando property?

Have you considered approaches not based on inheritance? I’ve worked on enough systems where inheritance-based extension mechanisms led to all kinds of fragile, hard-to-reason about messes to be very wary of that style. Would, maybe, something based on new ProseMirror view props that allow you to override certain subsystems (say, the renderer) with your own custom object be a viable route here?

3

Your PR mentions replacing the DOM mutation observer with beforeinput handlers. Do I understand correctly that you’re no longer parsing the modified DOM?

That’s correct

How do you reliably derive the appropriate changes from these events (which, on platforms like Android Chrome, tend to be fired in weird, incoherent batches)?

Ah, actually I don’t have an Android device and hadn’t realized that this was an issue! I’ll have to do some testing and see; it’s possible that this approach simply doesn’t work on platforms like Android Chrome with poor beforeInput support. I think that the beforeInput approach ended up not being strictly necessary; it should be possible to go back to using prosemirror-view’s DOMObserver in this case.

Edit: I just did some testing; I see what you mean! This approach does indeed fall apart on Android Chrome. I’ll spend some time tomorrow seeing how much work it would be to go back to the DOMObserver; I suspect the biggest challenge will be IME composition events (which I otherwise haven’t really accounted for; they seem to “just work” with beforeInput).

What do you use to replace the DOM-linked ViewDesc tree? Or are you still keeping a tree like that and adding it to a DOM node expando property?

We do in fact still keep a DOM-linked ViewDesc tree on an expando property. This ended up being necessary for compatibility with prosemirror-view; parts of the library expect to be able to pull the ViewDesc off of a DOM node, so we maintained that behavior. The ViewDescs themselves are slightly modified, however; they don’t have update methods (since React does the actual DOM maintenance) and some of the constructors have changed slightly, since we have different data available at different points in the ViewDesc lifecycle than the default EditorView implementation. I think this won’t matter; I don’t remember seeing anywhere outside of the ViewDesc’s themselves that checked instanceof ViewDesc.

Have you considered approaches not based on inheritance? I’ve worked on enough systems where inheritance-based extension mechanisms led to all kinds of fragile, hard-to-reason about messes to be very wary of that style. Would, maybe, something based on new ProseMirror view props that allow you to override certain subsystems (say, the renderer) with your own custom object be a viable route here?

Absolutely. I agree with you on the fragility of inheritance-based extension systems. I started with this route because I was hesitant to add props that felt too specific to this use case, but I’m definitely happy to work on an approach that allows customization through EditorView props, rather than inheritance.

4

I’ve been working on this a bit more, and I have successfully reverted to prosemirror-view’s DOMObserver for detecting changes, and I have composition events partly working! There’s still an issue when adding a text node to a previously empty textblock, which has proven challenging, but compositions in existing text nodes are working.

1 Like
5

Ok, everything seems to be working correctly with the prosemirror-view DOMObserver, now! Thank you for calling that out Marijn.

I’m a little unsure of the best way to disable the EditorView’s DOM updates with a prop. I’ve paired down the modifications I needed to make to EditorView from what you saw in the PR; I’m now only overriding updateStateInner. Here’s the overridden implementation I’m using:

  updateStateInner(state: EditorState, _prevProps: DirectEditorProps) {
    this.editable = !this.someProp(
      "editable",
      (value) => value(this.state) === false
    );

    const previousState = this.state;
    this.state = state;

    const scroll =
      previousState.plugins != state.plugins && !previousState.doc.eq(state.doc)
        ? "reset"
        : // @ts-expect-error scrollToSelection is internal
        state.scrollToSelection >
          // @ts-expect-error scrollToSelection is internal
          previousState.scrollToSelection
        ? "to selection"
        : "preserve";

    const updateSel = !state.selection.eq(previousState.selection);

    const oldScrollPos =
      scroll == "preserve" &&
      updateSel &&
      this.dom.style.overflowAnchor == null &&
      storeScrollPos(this);

    if (scroll == "reset") {
      this.dom.scrollTop = 0;
    } else if (scroll == "to selection") {
      this.scrollToSelection();
    } else if (oldScrollPos) {
      resetScrollPos(oldScrollPos);
    }
  }

I’d love your input on what you think would make the most sense here! I’m having trouble seeing a straightforward way to flag off parts of the existing updateStateInner so that when the disableUpdates prop (or whatever we call it) is passed we can get something like the implementation I’ve posted above, without restructuring the method a bit, which I don’t feel very confident in my ability to do safely.

6

Nice work! But must be tedious. Hope you have a test suite set up, could be crazy to figure out bugs otherwise

7

Thanks! It wasn’t/isn’t tedious, per se; the work has been super engaging and enjoyable. It’s definitely challenging though! Luckily, I think it’s now in a place where there’s essentially one component for each ViewDesc type (Widget, Mark, Node, TextNode), and then some code for ensuring that inline views are laid out in the correct hierarchy, etc., all of which feels very manageable.

But yes, testing has been very important and valuable! We’ve actually copied over large portions of the prosemirror-view test suite (over 100 test cases, and I’m going to add some more today!) to ensure that we have the same behavior as the default EditorView implementation.

1 Like
8

Will this work with React concurrent features? I’ve seen lots of ProseMirror code in the wild that relies on the DOM having been updated after dispatch returns…

Is there an online demo available? Would like to profile it to check keystroke latency and other performance characteristics…

9

Solving for React’s asynchronous render/commit cycle was the driving force behind this change, actually! I think even using Suspense boundaries should work with this approach. We’ve been trying different approaches to solving this problem since React 16, and I think this is going to prove to be the best. If you have any examples of ProseMirror code that relies on the DOM being synchronously updated after dispatch, I’d love to see them so I can see how this behaves.

And yes, actually there is! Please keep in mind that this code is still under review and is changing as we clean it up and improve it, so this demo may not work perfectly yet and some things may change over time, but there’s a live demo here: React-ProseMirror Demo. I haven’t done any performance profiling yet, so if you find anything interesting, please don’t hesitate to share!

10

All the plugin-related stuff seems to be gone. Is that intentional? Does your view not support plugins?

What is your solution for composition, precisely? The prosemirror-view approach is the way it is because touching the DOM near the cursor, or the selection, when a composition is active will usually disrupt it (or cause the browser to go to pieces entirely and start duplicating text or similar). Thus, it ‘freezes’ the text node with the cursor and its ancestors as long as the composition lasts, unless another transaction changes the text in there, in which case it gives up and overwrites the composition. I’d imagine you’ll have similar issues in React.

Do you have any approach to avoiding the race conditions that asynchronous React updates expose you to? The reason ProseMirror view updates are synchronous is that it allows it to treat the DOM and the editor state as in sync, making it possible to map DOM/screen positions to document position, to diff the DOM state to the current state on DOM mutation, and so on. If you introduce asynchronicity in there, that feels scary.

1 Like
11

All the plugin-related stuff seems to be gone. Is that intentional? Does your view not support plugins?

It does! Plugins are implemented in React hooks, rather than in the EditorView, in part to handle the asynchronicity of React DOM updates, which I’ll get into more below. There’s a hook called usePluginViews that does plugin view management.

What is your solution for composition, precisely?

Almost exactly as you’ve described, and I’m in the process of adding your test suite for composition to ensure parity. When there is an ongoing composition, the text node with the cursor “freezes” as long as the composition lasts. This was straightforward for existing text nodes and challenging for a composition that also creates a new text node, but the latest code handles both scenarios by having the parent NodeView component be responsible for freezing/maintaining the new text node created by the composition.

Do you have any approach to avoiding the race conditions that asynchronous React updates expose you to?

Absolutely! This is one of the key use cases for this library even before this change. This is an issue any time React is used, to be clear, so it’s something we were handling before as well (though it’s considerably simpler now). This library does not directly expose access to the EditorView; instead it exposes hooks, like useEditorEventCallback and useEditorEffect, that allow you to register event handlers and side effects that are guaranteed to only be called after React has completed updating the DOM, and therefore the DOM and the state are in sync. This allows developers to interact with the view in a way that feels idiomatic in a React context, while also ensuring that this access is safe!

Edit: Actually, now that I’m thinking about it, it seems likely that the scroll-to-selection management should also be happening in an effect to ensure that the DOM has been synced first, in which case it’s possible that what I need is simply a flag that reduces updateStateInner to just:

    this.editable = !this.someProp(
      "editable",
      (value) => value(this.state) === false
    );

    this.state = state;

I’d definitely love to get your thoughts on an approach like that!

12

I hope you kept the part using beforeinput events instead of mutation observers in a branch somewhere. The solution you describe should currently work most places, except IME/Android. Once EditContext ships in Chrome (supposedly later this year) the approach should even work on IME/Android (on Chromium). So unless browser makers change directions in the last minute, as they have done in the past, this approach should be the way all editor apps work in the near future.

13

I think, now that the DOMObserver implementation is working, I’ll rely on Marijn for that determination! We’ll try to keep the scope of React ProseMirror limited to DOM updates; if prosemirror-view eventually switches to a beforeinput-based input detection mechanism, then it’ll make its way downstream to us as well.

That’s awesome to hear about better beforeinput support in Android coming soon, though!

14

After a few years working directly with ProseMirror and React integration, I realised there is nothing close to solve the amount of edge cases the prosemirror-view already solves.

Now, with React 17+ changing the way events are handled I hope we could end up with a good solution to render custom NodeView and decorations using React without detaching from the main React tree.

15

Also, due to the lack of any official consensus how the contentEditable behaves during Selection changing. I have some concerns to let React managing it.

But, I am interested. I will spend some time this week checking out this PR. Nice work!!

16

That sounds like some really huge architectural changes. How is it possible to continue exposing the EditorView class to client code when it works completely differently? Won’t code written for plain ProseMirror make assumptions (synchronicity, plugin behavior, etc) that don’t hold on your version?

Or even, how does the code in prosemirror-view that you’re reusing still work in this scenario? Are the event handlers in input.ts still part of your setup? How is that safe, with the added asynchronicity?

17

How is it possible to continue exposing the EditorView class to client code when it works completely differently? Won’t code written for plain ProseMirror make assumptions (synchronicity, plugin behavior, etc) that don’t hold on your version?

I don’t think it’s accurate to say that it works completely differently, necessarily. There is one (significant!) difference, which is that DOM updates will be committed asynchronously, rather than synchronously. I think that the actual implications of this are fairly limited; it would be unsafe to synchronously inspect the DOM after dispatching a transaction and assume that the effects of the transaction have been committed, but this isn’t a common practice in ProseMirror library or third party code. Instead, DOM interactions tend to be limited to event handlers and plugin views, both of which are safe in this model.

The library also exposes the EditorView via the aforementioned React hooks. For example, one could write code that looks like:

export default forwardRef(function ParagraphWithTooltip({nodeProps, children, ...props}, ref) {
  const tooltipRef = useRef<HTMLDivElement | null>(null)

  useEditorEffect((view) => {
    if (!view || !tooltipRef.current) return

    // It's safe to use the EditorView to inspect the DOM here, because this runs
    // after DOM updates have been committed.
    const coords = view.coordsAtPos(nodeProps.pos)
    tooltipRef.current.style.top = `${coords.top}px`
    tooltipRef.current.style.left = `${coords.left}px`
  })

  return (
    <p ref={ref} {...props}>
      <div contentEditable={false} ref={tooltipRef} style={{position: 'absolute'}} />
      {children}
    </p>
  )
})

I also want to emphasize that this proposal is not treading new ground on this front. All existing React/ProseMirror integrations introduce this complexity. This includes TipTap, ReMirror, and the currently-published version of React ProseMirror. React ProseMirror already clearly documents this limitation, and will continue to do so. I expect that there are many developers for whom the tradeoff is worth it, even if it does mean that there might be some third party ProseMirror libraries that don’t work correctly (though, to be clear, I haven’t actually run into any of this yet).

… how does the code in prosemirror-view that you’re reusing still work in this scenario? Are the event handlers in input.ts still part of your setup? How is that safe, with the added asynchronicity?

They are, and they work! As far as I’ve seen, the event handlers in input.ts don’t ever dispatch a transaction and then synchronously inspect the DOM, expecting the results of that transaction to have been committed (hopefully I haven’t just missed something here).

18

I’m not sure about that—most of them seem to leave ProseMirror to handle its document updates, and only use React inside widgets and such. Thus, most of the relevant parts of the DOM will still be written synchronously. I guess you could have issues with layout moving once the widgets finish rendering.

I guess you are right that you can write most typical ProseMirror code without being impacted by the added asynchronous behavior. Though I’m sure people will occasionally do something like immediately find a DOM position after a dispatch to position a tooltip or something.

But what seems more worrying is user events. Say, the user does something that generates an editable DOM change, right between the time where something else was dispatched and the time at which React performs the DOM update. Now you have a changed DOM that started from an old editor state. In diffing and interpreting that, the current domchange.ts code would get confused, because it assumes the DOM started out synced with the editor state.

Anyway, back to plugins. Where are you performing the plugin updates now if updateStateInner doesn’t do it? I can imagine allowing the call to docView.update to be overridden somehow, but stuff like the cursor wrapper and plugin updates aren’t directly tied to that, yet also seem to need to be handled differently in your approach.

19

But what seems more worrying is user events. Say, the user does something that generates an editable DOM change, right between the time where something else was dispatched and the time at which React performs the DOM update. Now you have a changed DOM that started from an old editor state. In diffing and interpreting that, the current domchange.ts code would get confused, because it assumes the DOM started out synced with the editor state.

We should draw a distinction between deferred updates that React lets users opt into with special hooks and the batching of updates that are still committed immediately before yielding to the browser event loop.

When @SMores says that it’s not safe to inspect the DOM right after a dispatch and expect to see the result, that only applies to the current call stack. React leaves room for multiple state changes to get batched together during the same task, e.g. an event handler, but flushes/commits those changes to the DOM all together before yielding back to the browser event loop.

So, returning to the quote, there should never be a time when the user can do something interactive between a dispatch and the corresponding DOM update.

1 Like
20

React leaves room for multiple state changes to get batched together during the same task, e.g. an event handler, but flushes/commits those changes to the DOM all together before yielding back to the browser event loop.

An example of a deferred updates, as opposed to batched, immediate updates, would be the useDeferredValue hook. It would probably be a very bad idea to store the EditorState in a deferred value, but that’s true of all inputs (not just contenteditable / ProseMirror). The existence of deferred values in React is precisely to defer things that are not important to show immediately so as not to slow down inputs. Users want to see input reflected immediately, and React honors that, and makes it possible to defer other state changes.