Move rendering responsibility fully into React

[smoores-dev]

This is a re-imagining of this project's integration with ProseMirror's EditorView. Instead of attempting to carefully coordinate between React and ProseMirror, this fully moves the responsibility of rendering the editor state to the DOM into React.

How did this work before?

The primary work of this library is to manage the relationship between React and ProseMirror's EditorView. As laid out in The Problem Statement in the README, this is a fairly delicate relationship, and it's not without its tradeoffs. A short review:

• React updates in two phases (a side-effect-free state update phase, followed by a side-effect-ful render phase), while ProseMirror updates state and renders "all at once" (in that the two steps can't be separated)
  • This makes it challenging to store ProseMirror state in React; ProseMirror's state can't be updated until "phase 2", because it will execute side effects, so React will have "newer" state than ProseMirror during phase 1
  • Again, since ProseMirror renders synchronously after a state update, and React does not (at least, it's not guaranteed to), it is challenging to use React components to render ProseMirror node views, because ProseMirror asks for the rendered DOM from its node view constructors synchronously, and React can't provide that.

Until now, this library has relied on the default behavior of ProseMirror's EditorView for rendering non-node-view parts of the ProseMirror document, and has attempted to mitigate the challenges stated above by:

• Carefully coordinating state update timing so that we don't violate React's expectations about side effects
• Limiting access to the EditorView to times where we can be confident that its state is up to date with React's
• Wrapping all React-based node views with synchronously rendered DOM elements that can be handed to ProseMirror's EditorView during its one-phase update cycle.

Why are we interested in alternative solutions?

The current implementation is quite good at what it does, and it's proven to be very valuable. However, there are some downsides:

• Wrapping all React-based node views is unfortunate.
  • It can make it challenging to produce valid DOM in some scenarios. For example: If using React-based node views to render a ul with child lis, the actual ul and li elements have to be managed outside of React, in the constructor, and set as the contentDOM and DOM respectively.
  • It can make styling challenging, especially because one of the wrapping elements (the contentDOM wrapper) is produced internally to react-prosemirror and cannot be customized easily by consumers
  • It can introduce corner cases in contenteditable implementations, such as Deleting the last node view can throw errors #42 (which we managed to sort of hack around here, thanks again to @saranrapjs for figuring this out!)
• We induce a double render for every ProseMirror state update.
  • The order of events looks like:
    • State is updated through a dispatchTransaction call
    • The ProseMirror component is rendered, along with all of its children
      • This includes the React-based node view components, but their state is still the previous state
    • The EditorView is updated (in a useLayoutEffect). This is when the node views are created/updated, but the React-based ones are not synchronously rendered.
    • React-based node views are rendered again, this time with the new state
  • This is unintuitive from a developer standpoint, particularly the fact that React-based node views can be rendered with old state, even if they were the source of the state change (see controlled components in a createPortal from within a React NodeView behave weirdly #69)

What does this approach do differently?

This PR proposes to take over user input tracking (manual text input, at least) and rendering responsibilities from ProseMirror's EditorView, consolidating all rendering responsibilities into React.

This has several advantages:

• There is no longer any need for wrapping React-based node views in several layers of manually managed DOM elements. A React-based node view that renders only a <p> tag will result in only a <p> tag in the document.
• There is no longer any need for a double render. React-based node views will be immediately updated with any state changes, and the EditorView state will always be in sync with the React state.
• The interface for React-based node views is a little more idiomatic and hopefully easier to reason about.
• Widget decorations can also be authored with React components

The notable disadvantage to this approach is that non-React node views and widgets will have to be wrapped in a single wrapping element. This is less cumbersome than the React-based node view wrappers in the current approach, but is worth calling out.

How does it work?

This PR extends the EditorView class and overrides its NodeView and DOMObserver properties. In their places, we provide custom NodeViews, that act exactly the same as the default implementation, but execute no-ops when their update methods are called, and a custom DOMObserver that only listens for selection changes, and ignores DOM mutations.

We then introduce a beforeinput event handler that produces ProseMirror transactions based on the events it receives, which replaces the DOMObserver functionality that we eschewed in our selection-only implementation.

We also introduce a new React component suite, one for each type of NodeView, to replace the truncated update methods. When state is updated, we render a DocNodeView component with view.state.doc and the document decorations. Each NodeView component is responsible for rendering its children, so this eventually renders the entire ProseMirror document (and all of its decorations) in a single React tree.

[saranrapjs]

here we eschew prosemirror-view's EditorView entirely, in place of a custom-built React component

it doesn't look like any of the implementation here required changes to any of the existing React wrappers for ProseMirror view components, yes? is there a model where the React-based EditorView is offered as an additional import, but where we could commit to keeping compatibility with the regular ProseMirror EditorView? i can imagine scenarios where retaining that compatibility is important, presuming it doesn't really burden this library to do so

[smoores-dev]

@saranrapjs

it doesn't look like any of the implementation here required changes to any of the existing React wrappers for ProseMirror view components, yes? is there a model where the React-based EditorView is offered as an additional import, but where we could commit to keeping compatibility with the regular ProseMirror EditorView? i can imagine scenarios where retaining that compatibility is important, presuming it doesn't really burden this library to do so

I think that I wouldn't be in favor of that; there's almost no overlap between the two approaches, so we'd basically be maintaining two separate libraries. What kind of compatibility requirement are you imagining, though?

[saranrapjs]

What kind of compatibility requirement are you imagining, though?

@smoores-dev I'm picturing:

• Changes to non-prosemirror-view classes that depend on corresponding changes in prosemirror-view
• Changes other libraries depend on behavior we may not have implemented to spec in our React-based prosemirror-view
• Bugfixes in prosemirror-view that introduce corner cases in our implementation

Maybe this is a question we can ask Marijn? e.g. whether there's a "safe" way we should go about maintaining an EditorView implementation that tries to stay in sync with other prosemirror-* libraries?

[smoores-dev]

@saranrapjs Gotcha. Yeah I suspect that when it comes down to it, it's going to be a tradeoff between the risks you've outlined here, and the risks of the approach we have now (which mostly center around the wrapping elements). We've already run into several frustrating corner cases with the wrapping elements: #42 is one of these, and #26 is an attempt to mitigate a few others. I actually haven't decided which way I personally fall on this; there are obvious downsides to both options.

[smoores-dev]

@tilgovi @saranrapjs (and anyone else interesting):

So... what do you think? Should we actually try to do this? I don't see any reason that we literally could not continue implementing the rest of the EditorView API, though we might have to be thoughtful about how to handle, for example, commands that require the view (even joinBackward relies on the view, which I was kind of shocked to discover!).

I started down this road after #42; there may be a direct kludge (or even more elegant solution) that can solve that literal case, but it made me worried that there are more challenges with the general approach of wrapping elements that we initially expected.

I think the challenge with this approach, as @saranrapjs has pointed out, is figuring out how to ensure that whatever we do here actually plays nicely with existing prosemirror libraries, including user-contributed ones. In particular, this... might mean some support for plugins with a view prop...? But that also seems a little too goofy for me; maybe it's acceptable to say that we only support state-only plugins, and the whole reason to use React is so that it can own the entirety of your view.

[tilgovi]

But that also seems a little too goofy for me; maybe it's acceptable to say that we only support state-only plugins, and the whole reason to use React is so that it can own the entirety of your view.

That seems reasonable to me.

[tilgovi]

Unless we want to pursue compiling a secondary ReactDOM renderer that we can flush synchronously, I'm pretty ready to give up on trying to let ProseMirror own the DOM structure.

[tilgovi]

Unless we want to pursue compiling a secondary ReactDOM renderer that we can flush synchronously, I'm pretty ready to give up on trying to let ProseMirror own the DOM structure.

And didn't you do some experiments with that from which you concluded even that had problems?

[smoores-dev]

And didn't you do some experiments with that from which you concluded even that had problems?

Indeed; secondary renderers can't share context with primary renderers, so context has to be manually bound across the renderer gap, and each node view is in its own private react element tree, so you lose the legibility in dev tools (and you can't pass context from parent node views to child node views). Some more info here for anyone following along: https://discuss.prosemirror.net/t/announcing-react-prosemirror/5328/13

[saranrapjs]

(I am going to ask Marijn about this and will try to ferry any notable details from that conversation back here!)

[smoores-dev]

Alright, thanks @saranrapjs! If you think he'd be open to it, could be neat to have Marijn join the conversation here, too!

[saranrapjs]

quoting Marijn:

e.g. where a bug fix requires a change to prosemirror-state and prosemirror-view, or prosemirror-tables and prosemirror-view (or something like that)

This is very unlikely. I keep each of these packages backwards-compatible, by itself, and in cases where some old behavior has to be replaced I've set things up so that the old thing also remains available, but peer packages most to using the new thing.

As for a full parallel implementation of prosemirror-view, I very much share the concern voiced in the PR that this will have to duplicate a lot of finnicky, fragile code (about 6000 lines), which will be a pain to get right and maintain.

I think without a plan to account for something like prosemirror-view's test suite, for example, I think I'd frame my POV as: we should only do this we can avoid breaking compatibility for library consumers that still want to use the mainline prosemirror-view.EditorView.

[smoores-dev]

we should only do this we can avoid breaking compatibility for library consumers that still want to use the mainline prosemirror-view.EditorView.

I don't totally understand what this would mean, I don't think. Are you suggesting two different APIs exposed by this library, the current one and the one proposed by this PR?

[saranrapjs]

Are you suggesting two different APIs exposed by this library, the current one and the one proposed by this PR?

Is there any code on this PR that breaks react-prosemirror-managed NodeView functionality currently working with ProseMirror's EditorView? I see reimplementations of EditorView-isms, and a couple of classes (NodeWrapper, TextNodeWrapper) that maybe point to functionality that overlap with some of what useNodeViews does today — but is there code here that breaks the existing code used to manage NodeView placements? (maybe this would come further when you'd need to change the API "exposed" to NodeView React components to be more React-y — like some of what's in Randall's snippet?)

[smoores-dev]

Nothing about the original API (the <ProseMirror /> component and the useNodViews hook) is broken by the new <EditorView /> component, but it is replaced by it. That is, aside from the LayoutGroup system, there's no overlap between the two interfaces, and I can't imagine how we could continue to expose both without it being very confusing for consumers (they're not interchangeable, you have to exclusively use one or the other). Essentially none of the old hooks, aside from maybe useEditorState, make much sense in the context of the <EditorView /> component; and meanwhile there will need to be several new hooks (useDomAtPos and useCoordsAtPos, e.g.) that will not work/make sense in the old system.

Basically, I think we have to pick. I don't think this library can meaningfully support the actual EditorView class if we go with an all-React rendering approach. But I'm not at all opposed to attempting to run prosemirror-view's test suite (or at least a version of it) here to try to give us more confidence about compatibility.

I also want to be clear that I would much rather not be in a position where this feels like an approach we have to consider, but at this point we've been struggling to make React-based NodeViews interop with prosemirror-view for several years, and while I think react-prosemirror is the closest we've ever gotten, it still has some hurdles that feel increasingly gnarly. Basically, I'm saying that I don't think we're picking between "It works generally and also works with native EditorView" and "It probably works generally and doesn't work with native EditorView"; rather the first option is more like "It sort of works if you're very careful and we don't have a clear path toward it actually working robustly".

[saranrapjs]

Essentially none of the old hooks, aside from maybe useEditorState, make much sense in the context of the component; and meanwhile there will need to be several new hooks (useDomAtPos and useCoordsAtPos, e.g.) that will not work/make sense in the old system

I think I'm not seeing how this is confusing — these seem like non-overlapping but mutually compatible code entrypoints we could maintain in parallel if we want to.

Basically, I think we have to pick.

I think I'm still not totally convinced that we do? To put it another way: if we want to make a bet that we can get a compatible implementation of EditorView working in React and stable enough that it provides value to users of this library such that they opt to switch to react-prosemirror's EditorView, that seems like a totally worthwhile bet to make. But this library provides value now, even where as you note there are plenty of caveats centering around "it sort of works if you're very careful" that means that it's not where we might like it to be.

My feeling is that if there's a path to evaluating the stability of a react-prosemirror EditorView that doesn't force library consumers to choose between stability and robustness while we're still trying this out, we don't lose much in maintaining compatibility for some period of time. Unless you're picturing that this branch might be relatively long-lived while we work out all of the kinks? If you think it's reasonable that we wouldn't merge a branch like this until we'd have something like "compatibility with prosemirror-view's test suite", that might be all I'm asking about here; I wouldn't feel as strongly about committing to avoiding breaking compatibility if we can commit to a way to measure our confidence.

[smoores-dev]

Ah, I think I see where I was misunderstanding you. I had been interpreting your previous messages as a suggestion that we should indefinitely maintain both interfaces.

If you think it's reasonable that we wouldn't merge a branch like this until we'd have something like "compatibility with prosemirror-view's test suite", that might be all I'm asking about here; I wouldn't feel as strongly about committing to avoiding breaking compatibility if we can commit to a way to measure our confidence.

This is, in fact, exactly what my plan was, and I should have made that clearer upfront! I have no intention of merging this branch (or at least, exporting/documenting any of the new functionality) until we have a way to measure our confidence. I only opened this draft at this stage so that I could get a sense of whether this felt like a worthwhile endeavor to you all before I sink any more time into making it more robust!

[tilgovi]

I don't want to rush this branch but I think this library provides very little value as it is now. It has some nice ideas, but without the stuff we've done internally at the Times to make simple node views work without wrappers, I'm skeptical that it's very useful to many people.