react-prosemirror
react-prosemirror copied to clipboard
nytimes
Move rendering responsibility fully into React
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
ulwith childlis, the actualulandlielements 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 #42 (which we managed to sort of hack around here, thanks again to @saranrapjs for figuring this out!)
- It can make it challenging to produce valid DOM in some scenarios. For example: If using React-based node views to render a
- We induce a double render for every ProseMirror state update.
- The order of events looks like:
- State is updated through a
dispatchTransactioncall - 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
- State is updated through a
- 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 #69)
- The order of events looks like:
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.
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
@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?
What kind of compatibility requirement are you imagining, though?
@smoores-dev I'm picturing:
- Changes to non-
prosemirror-viewclasses that depend on corresponding changes inprosemirror-view - Changes other libraries depend on behavior we may not have implemented to spec in our React-based
prosemirror-view - Bugfixes in
prosemirror-viewthat 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?
@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.
@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.
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.
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.
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?
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
(I am going to ask Marijn about this and will try to ferry any notable details from that conversation back here!)
Alright, thanks @saranrapjs! If you think he'd be open to it, could be neat to have Marijn join the conversation here, too!
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.
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?
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?)
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".
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
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.
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!
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.
It sounds like there's enough interest in this approach to continue forward; I'm going to do my best to continue working on this direction, porting over code from prosemirror-view as needed (the coordsAtPos code for example is almost directly from prosemirror-view), and see if I can't match the prosemirror-view test suite as well, to add some confidence.
If anyone has any time to jam, my plan is to work on this every Friday for the foreseeable future, and I'd be very open to some pairing sessions!
@tilgovi (et al) the latest here is in sort of a nice place, I think, or at least heading there. I've directly copied over all of prosemirror-view's domcoords.ts, dom.ts, and browser.ts, and am simply passing the EditorView API implementation that we're building in EditorView.ts as the view argument to those implementations (suddenly very happy those are entirely implemented as standalone functions that take a view as an argument, rather than class methods!).
I've also copied over slightly truncated versions of the ViewDesc classes (removing anything related to update or destroy, since that's React's job now). I suspect that some parts of ViewDesc could be simplified if they relied on posToDesc, rather than the more elaborate position computation system that they're currently using, but it's working as is so I'm tempted to leave that for later, if at all.
There's a new useView hook that provides access to the view API. The restrictions are basically the same as the implementation on main: you can only safely access the view API in an effect or event handler (otherwise the parts of the view API that access the DOM may not be up-to-date).
I feel much, much, much better about this now that the ViewDesc classes and dom.ts/domcoords.ts code is working nearly as-is from prosemirror-view. I'd like to think about what someProp means in this system, and take a close look at prosemirror-view's input.ts and selection.ts and try to figure out how much of that needs to exist in this system, and how much should me managed with React instead.
@tilgovi @saranrapjs
Very big update
As I was going, my branch was increasingly looking like a chaotic fork of prosemirror-view, where I was hacking around, replacing the DOMObserver with a beforeinput listener, and replacing EditorView.update with my React components.
So I decided to lean into it, and spent some time today making it a much less chaotic fork of prosemirror-view. The diff from prosemirror-view is now as small as I could get it:
- I'm deferring the creation of the
docNodeView, which prosemirror-view does in theEditorViewconstructor, to happen in some React useEffects. - I've overridden
EditorView.updateState,EditorView.updatePluginViews, andEditorView.setPropsso that they no longer try to mutate the DOM. - I've overridden the DOMObserver implementation so that it's only responsible for managing the selection, rather than actually observing the DOM with a MutationObserver
These first two changes allow React to be entirely responsible for translating the state into the DOM. The third change allows us to use a beforeInput observer for translating user inputs into ProseMirror transactions, rather than observing how the DOM changes and working backwards, which breaks React's rendering model (because the DOM is changed out from under it).
Similarly, I've limited the scope of prosemirror-view internals that the branch is using as much as possible. I'll have to do another pass to be sure, but I believe I'm now only using:
selectionToDOM, to ensure that react-prosemirror places the DOM selection in the same place that prosemirror-view would for any given ProseMirror Selection.- A very lightly forked implementation of
iterDeco, to ensure that we render widget and node decorations in the same places that prosemirror-view does. - The ViewDesc class, and its subclasses. I have forked these for ease of use here, but I believe that could also use them as-is!
These changes are so small that I think we should try to broach the conversation with Marijn about whether they could be upstreamed into prosemirror-view. With the exception of exposing iterDeco, selectionToDOM, and ViewDesc, the only changes that have to happen in prosemirror-view itself are:
- Allow consumers of EditorView to set the docView themselves, somehow, rather than the default implementation
- Allow consumers of EditorView to override the default DOMObserver
- Allow consumers of EditorView to opt out of EditorView.updatePluginView (this is optional; we could work around this if we had to).
The other changes could be accomplished by subclassing EditorView to override method implementations, like updateState and setProps.
I'm going to try to summarize the state of this pull request; a lot has happened here since this was first suggested as an approach!
Firstly, the elephant in the room: This PR includes a fork of the prosemirror-view library, in the directory prosemirror-view. This is obviously not sustainable, and we shouldn't merge this PR with this fork! The diff between the fork and mainline prosemirorr-view is actually quite small, and I've called out everywhere that I actually changed the code of prosemirror-view with a $$FORK comment. This week I'm hoping to finish up work on going back to the DOMObserver, rather than the custom beforeInput handler currently on this branch (more on this below), and thereby make the diff even smaller. The explicit goal is to come up with a minimal tweak to the EditorProps interface that can be upstreamed into prosemirror-view that will support this work without an additional maintenance burden on this library or prosemirror-view.
TL;DR - There's a fork of prosemirror-view here now, but we want to upstream the changes into the library before merging this branch. We won't merge this branch while it still has a fork of prosemirror-view; it's only there for development and experimentation.
On to the actual approach:
This PR essentially replaces prosemirror-view's ViewDesc.updateChildren with a React component tree. Everything else is the same, which is to say that this approach continues to use an actual instance of the EditorView for everything else that the EditorView does, including DOM observation, input detection and event handler execution, selection syncing, etc.
We do this by introducing some new React components, such as DocNodeView, NodeView, TextNodeView, WidgetView, and MarkView. Each of these is responsible for rendering some type of content represented by a view descriptor. That is, we have replaced the updateChildren method of each ViewDesc subclass with a React component with equivalent behavior.
There are some subtleties here, like the ProseMirror plugin that assigns unique keys to nodes to maintain React element stability across rerenders, that I'm happy to go into further detail about if anyone is interested.
The current status
This PR has been brought to Marijn's attention here. At his suggestion, I've been working on getting this approach working with prosemirror-view's DOMObserver implementation, rather than the custom beforeinput handler approach that I originally tried here. There are, unfortunately, still widely used platforms (e.g. Android Chrome) that simply don't have well-behaved beforeinput behavior. Luckily this seems to be working, and I have a local diff that I will push later this week using DOMObserver that mostly works, with a specific edge case around using composition events in a previously empty textblock.
My goal is to get this DOMObserver implementation up to 100% compliance with the test suite and current prosemirror-view behavior, and then propose a minimal change to the prosemirror-view library that will allow us to take this approach here!
Heads up: I just quite dramatically rebased the git history of this branch, which was at 77 commits and had gotten completely unwieldy. For folks taking a look at this branch for the first time: I recommend looking through the commits in order to get a sense of what's going on!
Is there a getting started guide or similar doc for this branch? Is this published anywhere as a package yet?
@plaisted yeah! The README on this branch has been updated to reflect the changes to the APIs that this branch makes, and it's published to npm under the next tag (the latest version is 0.7.0-next.5).