Fix bug with infinite rendering loop which can sometimes happen with widget decorations

[saranrapjs]

We've seen an issue where rendering a widget decoration into an empty ProseMirror node with an associated react-prosemirror NodeView causes an infinite loop: the NodeView's update hook is called before the component's ref is set, which causes update to return false, which in turn causes ProseMirror to destroy and recreate the NodeView, repeating the cycle. The test committed here should illustrate the bug.

I'm feeling OK about the fix to the bug, even while I'll admit that I'm not totally sure I understand why this is happening: it's important to tell ProseMirror that an update either did update a node (returns true) or did not (returns false). At this point in the NodeView component code we should have reasonable confidence that this NodeView is responsible for a node at this position, even while we don't have the ref yet and so can't confirm that the Node's type matches the one passed down into the React NodeViewWrapper. By returning true here, a subsequent update cycle is called once the ref has been set, and the logic proceeds as it has been.

Some trailing observations:

• I still don't know why "render widget decoration into empty node" causes this — maybe widget rendering ends up triggering the update method earlier in relation to ReactDOM's rendering cycle?
• I haven't confirmed whether this issue occurs on the v2.x flavor of react-prosemirror; my guess is that it probably does not, though, because the calling code responsible for destroying/recreating the NodeView in this scenario is non-React prosemirror-view.EditorView code.

[halfghaninne]

Are there any cases where the componentRef would stay null (maybe if the NodeView component fails to mount?) that we want to account for in this addition to the code?

[saranrapjs]

Are there any cases where the componentRef would stay null (maybe if the NodeView component fails to mount?) that we want to account for in this addition to the code?

I think if the component fails to mount due to error, this code will crash, so I'm not sure that we need to handle that scenario here (or if we did, we'd need different API's accounting for that possibility). I'm not aware of other situations which would cause the component to fail to eventually mount, but I'm still fuzzy enough on why the sequencing changes when a decoration widget is in play that I'm not sure that I can offer more confidence than that

[tilgovi]

I'm taking a look to see what's going on here. In a quick debugging session I'm seeing that the paragraph does register and unregister its ref (twice, actually, but that may just be an effect of React's dev mode) before the new code you've added ever runs. But it does look like the code you've added does run before the textblock registers its ref. I'd like to understand why the order of things if different before we add defensive checks that may mask other issues down the line or lead to unexpected states.

[tilgovi]

Oops, when I said paragraph I was failing to run only this test. In this test, there is no paragraph. I am seeing the behavior where the update runs before the ref registration, and I'm going to dig a little to see if I can identify why.

[tilgovi]

We use a mount ref to give users control over the editing host element. The way useEditorView is written, we have layout effects that are meant to run and re-render the editor with the editing host element mounted. That should be a noop update for ProseMirror, though, because both new EditorView(...) and setProps(...) should be called with the same props. However, the plugin isn't memoizing the decoration set, and I think that's forcing a ProseMirror update to happen before we've rendered the React tree. Gotta dig a little deeper, still.

[tilgovi]

With this change, the code you've added never gets called:

           return DecorationSet.create(state.doc, [
             Decoration.widget(1, () => {
               const div = document.createElement("div");
               return div;
-            }),
+            }, { key: "foo" }),
         },
       },

[tilgovi]

To summarize, I think this issue occurs when the decorations defined at the time that the editor mounts aren't keyed or otherwise memoized over state.

If we want to account for this, I think the fix should be in useEditorView to avoid the extra setProps(). I'm just not sure I'd want to do that, since it means putting logic in the hook to determine whether setProps needs to be called, when I'd rather just call it always and let ProseMirror determine what work needs to be done to update itself.

[saranrapjs]

To summarize, I think this issue occurs when the decorations defined at the time that the editor mounts aren't keyed or otherwise memoized over state.

The previous fix I was working on was indeed in calling code for the plugin this test case approximates, specifically to set a key for a plugin that wasn't otherwise setting one (for no particular reason).

I'm OK to close this PR and fix upstream if we think the specific situation of key-less widget decorations is uncommon enough, and would be complicating in the implementation to guard against in the v1.x react-prosemirror, as to not warrant fixing here. Though I think I'm not yet confident I understand the decoration memoization you're describing enough to know whether this would also happen for other decorations — e.g. even with decorations that don't support key? If this situation only happens for widgets, I'm cool with fixing this elsewhere.

[tilgovi]

I'm OK to close this PR and fix upstream

By this do you mean in the application using react-prosemirror, or fixing something in prosemirror itself?

[saranrapjs]

By this do you mean in the application using react-prosemirror, or fixing something in prosemirror itself?

I can fix in the application using react-prosemirror that includes the plugin not currently specifying key currently, if we think that this is a bug that's narrow to key-less widget decorations and not worth burdening the v1.x implementation.

(If this bug is possible with other non-widget decorations too, I might be more inclined to try to figure out a fix here)

[tilgovi]

Happy to consider fixing things so that keyless widgets don't cause infinite loops at mount. I just don't think this PR is the fix I'd want to commit.

[tilgovi]

I think we have a better understanding of the cause of the extra update cycle and why we end up doing a ProseMirror update at a moment in the React commit cycle that we didn't plan for. I would prefer to try to avoid that update or think a little harder about how to handle that update before committing a fix that could have unexpected effects in other scenarios.

I'm concerned about keeping the mental model here clear. We expect that React will render and then commit, and as a result refs will exist before ProseMirror updates. Anything we do to accommodate other behavior is inviting trouble and confusion, I think.

[smoores-dev]

I'm taking a look now to see if I can get a better sense of why, specifically, we're getting into a loop here. One thing I found, incidentally, is that ProseMirror View considers two widgets equal if they:

1. Have the same key
2. Are the same instance
3. Do not have a key, and have the same toDOM function

Which means that in addition to fixing this with a key, you can also fix it by pulling out the toDOM function into module scope. Kind of funny, and doesn't fix the underlying problem (widgets should not cause a crash even if they get rebuilt on every update). Gonna keep digging!

[saranrapjs]

If we want to account for this, I think the fix should be in useEditorView to avoid the extra setProps(). I'm just not sure I'd want to do that, since it means putting logic in the hook to determine whether setProps needs to be called, when I'd rather just call it always and let ProseMirror determine what work needs to be done to update itself.

I'm reflecting on this idea again...EditorView.setProps calls EditorView.update, which (per the docs) "Will immediately cause an update to the DOM". This seems weird! I'm wondering if a narrow fix in this situation if we could propose a patch in EditorView to do a cheap check whether incoming props has identity equality with existing props, and bail the update if they do — e.g. here? I could be misunderstanding the props changes (e.g. maybe there are changes to state here?) but I'd think that if props isn't actually changing at all, this might be a way to fix avoid extra update call.

[tilgovi]

Please have a look at #158 and close if you think that's a better way to go. After talking this through following our in person discussions today, I was able to figure out that pushing the node view portal management down into <ProseMirror> we're able to ensure that the extra render that happens after mounting the editor view includes the initial node views such that the refs are all filled. It's also a nice ergonomic upgrade, I think!

[tilgovi]

If we want to account for this, I think the fix should be in useEditorView to avoid the extra setProps(). I'm just not sure I'd want to do that, since it means putting logic in the hook to determine whether setProps needs to be called, when I'd rather just call it always and let ProseMirror determine what work needs to be done to update itself.

I'm reflecting on this idea again...EditorView.setProps calls EditorView.update, which (per the docs) "Will immediately cause an update to the DOM". This seems weird! I'm wondering if a narrow fix in this situation if we could propose a patch in EditorView to do a cheap check whether incoming props has identity equality with existing props, and bail the update if they do — e.g. here? I could be misunderstanding the props changes (e.g. maybe there are changes to state here?) but I'd think that if props isn't actually changing at all, this might be a way to fix avoid extra update call.

You could pursue this, for sure. I've thought about it, but haven't ever attempted it due to the way that ProseMirror mingles props and state. Tracking them separately would probably allow some optimizations in how ProseMirror decides what to recompute.

In any case, I think #158 would make such a change into an optimization rather than a bug fix and I'm eager to hear your reaction.