Using a cached NodeView causes a null error when updating decorations in some cases

[kiejo]

Since ProseMirror can often recreate a NodeView in cases where this is not wanted for my use case, I have implemented logic which allows me to cache and reuse NodeViews. The primary motivation is that a node view should keep its state as long as the same node is shown in the editor (e.g. wrapping an existing node in a list should not destroy and recreate the node view and make it lose its state).
One specific example is that a node view, which renders an iframe, should not reload the iframe when editing the document.

This is the first time I'm running into an issue with the custom node view caching and it was relatively difficult to reproduce. Here is the smallest possible reproduction I was able to come up with. Please note that this does not contain all of the node view caching logic, only what was needed to reproduce the issue. The code can directly be run here: https://glitch.com/edit/#!/mica-closed-lint?path=index.js

const {EditorState, Plugin} = require("prosemirror-state")
const {EditorView, Decoration, DecorationSet} = require("prosemirror-view")
const {Schema, DOMParser} = require("prosemirror-model")
const {schema} = require("prosemirror-schema-basic")
const {exampleSetup} = require("prosemirror-example-setup")

const mySchema = new Schema({
  nodes: schema.spec.nodes.append({
    "test": {
      inline: true,
      group: 'inline',
      toDOM(node) {
        return ['span', {}, 'INLINE-NODE']
      },
      parseDOM: [{tag: '[data-type="test"]'}]
    }
  }),
  marks: schema.spec.marks
})

// A plugin which applies inline decorations for a given array of ranges
const decoPlugin = new Plugin({
  state: {
    init() {
      return DecorationSet.empty
    },
    apply(tr, pluginState) {
      if (tr.getMeta("setDecos")) {
        const ranges = tr.getMeta("setDecos")
        return DecorationSet.create(tr.doc, ranges.map(({from, to}) => Decoration.inline(from, to, {style: "color: red"})))
      } else {
        return pluginState.map(tr.mapping, tr.doc)
      }
    }
  },
  props: {
    decorations(state) {
      return decoPlugin.getState(state)
    }
  }
})

const nodeViewCache = new Map()

class StaticTextNodeView {
  constructor(node) {
    nodeViewCache.set(node, this)
    this.dom = document.createElement("span")
    this.dom.textContent = "Static Text"
  }
  
  destroy() {
    // A view for the exact same node might get recreated right after destruction, which is why we add a delay
    // so we get the chance to reuse a cached node view before destroying it. If the node view is not re-created
    // shortly after destruction, we destroy it.
    setTimeout(() => {
      if (!this.dom.isConnected) {
        // The node view has not been remounted => Should be deleted as it's not part of the editor anymore.
        nodeViewCache.delete(this.node)
        // Perform destruction
      }
    }, 20)
  }
}

// Try to use a cached node view for the given node instead of always creating a new one
function cachedNodeView(createNodeView) {
  return (node, view, getPos) => {
    return nodeViewCache.get(node) || createNodeView(node, view, getPos)
  }
}

window.view = new EditorView(document.querySelector("#editor"), {
  state: EditorState.create({
    doc: DOMParser.fromSchema(mySchema).parse(document.querySelector("#content")),
    plugins: [
      ...exampleSetup({schema: mySchema}),
      decoPlugin,
    ]
  }),
  nodeViews: {
    "test": cachedNodeView((node, view, getPos) => new StaticTextNodeView(node))
  },
})

const tr = window.view.state.tr.setMeta("setDecos", [
  {from: 1, to: 2},
  {from: 3, to: 4},
  {from: 6, to: 7},
  {from: 9, to: 10},
  {from: 11, to: 12},
  {from: 23, to: 24},
])
window.view.dispatch(tr)

// Schedule a decoration change, which causes the uncaught error
setTimeout(() => {
  console.log('update decos')
  const tr = window.view.state.tr.setMeta("setDecos", [])
  window.view.dispatch(tr)
}, 100)

This is the error I get when updating the decorations in a specific way:

Uncaught TypeError: Cannot read properties of null (reading 'nextSibling')
    at rm
    at renderDescs
    at NodeViewDesc.updateChildren
    at NodeViewDesc.updateInner
    at NodeViewDesc.update
    at ViewTreeUpdater.updateNextNode
    at eval
    at iterDeco
    at NodeViewDesc.updateChildren
    at NodeViewDesc.updateInner

The issue only occurs when reusing the cached node view. My guess is that it has to be somehow related to the dom property of the node view and how it gets created or attached to the DOM.

[marijnh]

Your caching scheme is causing multiple nodes to use the same DOM element, which isn't supported and will break the editor in precisely the way you are seeing here. I verified by adding this loop to the start of renderDescs:

  let set = new Set
  for (let ch of descs) {
    if (set.has(ch.dom)) throw new Error("Duplicate")
    set.add(ch.dom)
  }

[kiejo]

Thanks for the quick response!

Based on my understanding, my caching scheme always returns the same NodeView instance and DOM element for the same node instance, but not for different node instances.
What seems to be happening is that the NodeView creation function gets called by ProseMirror for a node that already has an active NodeView and is already present in the DOM. In this case the existing NodeView along with its DOM is returned by the caching mechanism, which I did not expect to break anything.

What's interesting to me is that adding an id attribute to the Node seems to resolve the issue without applying any other changes to the code: https://glitch.com/edit/#!/cottony-antique-yogurt?path=index.js

Based on all this, I don't think there is a bug in renderDescs, but there might potentially be an issue with the way NodeViews get created. I might also be misunderstanding the way NodeViews get created and what constraints apply.
On the high level it would be very helpful to understand how to best prevent the unnecessary (at least from a user perspective) recreation of NodeViews without causing any unexpected side effects.

[marijnh]

Nodes are compared by content/type, not by identity. Using them as cache keys isn't going to be safe.

[kiejo]

Thanks for clearing that up, that explains the issue.