Undo stops working after late-registering another prosemirror plugin with yjs 13.5.32+ due to new destroy logic in yjs.

[ascott18]

Checklist

• Are you reporting a bug? Use github issues for bug reports and feature requests. For general questions, please use https://discuss.yjs.dev/
• Try to report your issue in the correct repository. Yjs consists of many modules. When in doubt, report it to https://github.com/yjs/yjs/issues/

Describe the bug
Scenario is as follows:

• Prosemirror initialized with yUndoPlugin() (default settings)
• UndoManager is subsequently setup with afterTransaction listener on the YDoc. Everything normal up to this point.
• An additional Prosemirror plugin is late registered. In my case, https://github.com/ueberdosis/tiptap/blob/v1/packages/tiptap/src/Components/EditorMenuBubble.js#L32, but this could be any plugin registered after the prosemirror EditorView has been initialized.
• When this plugin is late registered, the following stack trace occurs:

destroy (yjs.mjs:4848)                 -- yjs
destroy (undo-plugin.js:116)           -- y-prosemirror
destroyPluginViews (index.es.js:6758)  -- prosemirror-view
updatePluginViews (index.es.js:6765)   -- prosemirror-view
updateStateInner (index.es.js:6734)    -- prosemirror-view
updateState (index.es.js:6652)         -- prosemirror-view
registerPlugin (tiptap.esm.js:1396)    -- tiptap (Editor.js)
(anonymous) (tiptap.esm.js:1800)       -- tiptap (EditorMenuBubble.js)
... vue stuff

• UndoManager is now destroyed, and will not be created again by undo-plugin.

Expected behavior
The UndoManager should not be destroyed when the EditorView is destroyed, since the UndoManager is initialized by the plugin, not by the plugin's EditorView. The plugin itself and the plugin's EditorView have different lifecycles.

Initializing the UndoManager along with the EditorView (e.g. in view: view => { ... } is not a good solution, since it will cause undo state to be wiped out whenever a prosemirror plugin is registered. Imagine a prosemirror editor that gradually registered plugins as a user toggles settings, or one that only registers plugins on demand).

I think the best solution would be to just remove the destroy hook from the EditorView of undo-plugin. Just leave the lifecycle of the UndoManager tied to the lifecycle of the YDoc (which it already hooks into in UndoManager's ctor).

Environment Information

• Browser / Node.js [e.g. Chrome, Firefox, Node.js]: Chrome
• yjs 13.5.32, also tested against 13.5.38
• y-prosemirror 1.1.0

[dmonad]

This looks like a duplicate of #102, is that right?

It is basically not possible to distinguish between destroy and reconfiguration which makes this issue hard to deal with. I'd prefer to continue the discussion in the existing tichet. Feel free to close this one and reopen the other ticket.

[Alecyrus]

ueberdosis/tiptap#2761

[ascott18]

@dmonad Yes, it looks like the same issue.

[hamflx]

I had the same problem and this is how I fixed it:

import { Collaboration } from '@tiptap/extension-collaboration'
import { ySyncPlugin, yUndoPlugin, yUndoPluginKey } from 'y-prosemirror'

export const CollaborationExtension = Collaboration.extend({
  addProseMirrorPlugins () {
    const fragment = this.options.fragment
      ? this.options.fragment
      : this.options.document.getXmlFragment(this.options.field)

    const yUndoPluginInstance = yUndoPlugin()
    const originalUndoPluginView = yUndoPluginInstance.spec.view
    yUndoPluginInstance.spec.view = view => {
      const undoManager = yUndoPluginKey.getState(view.state).undoManager
      if (undoManager.restore) {
        undoManager.restore()
        undoManager.restore = () => {}
      }
      const viewRet = originalUndoPluginView(view)
      return {
        destroy: () => {
          const hasUndoManSelf = undoManager.trackedOrigins.has(undoManager)
          const observers = undoManager._observers
          undoManager.restore = () => {
            if (hasUndoManSelf) {
              undoManager.trackedOrigins.add(undoManager)
            }
            undoManager.doc.on('afterTransaction', undoManager.afterTransactionHandler)
            undoManager._observers = observers
          }
          viewRet.destroy()
        }
      }
    }
    return [
      ySyncPlugin(fragment),
      yUndoPluginInstance
    ]
  }
})

[LeeSanity]

yUndoManager constructor has an optioanal undoManager parameter, which supports init an external undoManager instance and pass in. In our use case, we put many prosemirror editors and some other components into one Y.Doc, and use a global undoManager to listen changes, in this way, even some editor deleted from the dom (Y.Doc), we should't destroy the undoManager instance. So... I came up with two approches:

• add some logic in yUndoPlugin, if the undoManager passed in as a parameter, then we should't destory it anyway
• maybe add some optional parameter for undoManager, and leave the destory logic to the users

what do you think? @dmonad

[dmonad]

Hi @LeeSanity,

Let's go with the second approach to keep compatibility. Can you please create a PR that adds a parameter (shouldDestroyUndoManager) with some documentation? Happy to assist & merge.

[hamflx]

This PR yjs/yjs#449 mentioned by @LeeSanity works fine for external UndoManager, but for internally created UndoManager, if shouldDestroyUndoManager is set to true, then there is still this problem, otherwise set to false, then the user needs to destroy the UndoManager instance manually..

I provide a solution here to solve both problems, implement a UndoManagerDelayedDestroy class inherited from UndoManager, providing two methods: preventDestroy and delayedDestroy. The delayedDestroy method is used to call super.destroy method in the next tick to destroy the UndoManager instance. The preventDestroy method is used to prevent the destruction.

see: hamflx@fd79c58

The code is as follows.

// replace "new UndoManager"
// const _undoManager = undoManager || new UndoManagerDelayedDestroy(ystate.type, {

// Call undoManager.preventDestroy at the beginning of the view function
// const undoManager = yUndoPluginKey.getState(view.state).undoManager
// undoManager.preventDestroy()

// Call `undoManager.delayedDestroy` instead of `undoManager.destroy`.
//     if (typeof undoManager.delayedDestroy === 'function') {
//       undoManager.delayedDestroy()

class UndoManagerDelayedDestroy extends UndoManager {
  constructor (type, opts) {
    super(type, opts)
    this.destroyCounter = 0
  }

  preventDestroy () {
    this.destroyCounter++
  }

  delayedDestroy () {
    const memorizedCounter = this.destroyCounter
    queue(() => this.destroyCounter === memorizedCounter && super.destroy())
  }
}

const queue = fn => Promise.resolve().then(fn)

• The external UndoManager does not have preventDestroy and delayedDestroy methods, so nothing happens.
• The internal UndoManager, will call delayedDestroy first and then be cancelled by the subsequent preventDestroy unless the view is actually destroyed for the last time.

Translated with www.DeepL.com/Translator (free version)

[LeeSanity]

You are right @hamflx , my proposal just works for external UndoManager use cases. For internally created UndoManager, I think it's the problem between prosemirror editor & yUndoPlugin. There lacks one way to detect plugin reconfig.

[hamflx]

If there is no better way, is it possible to consider my proposal?

@dmonad

[shlroland]

@dmonad @hamflx @LeeSanity hi,Is there any progress on this issue? I have also encountered this problem while using remirror and I hope that an official fix will be available soon.

[jsonkao]

Hi, I also encountered this issue — I was sharing an UndoManager across multiple Prosemirror editors and it was being accidentally destroyed. I used a simplified version of @hamflx's fix to fix this (below), but it would be nice for a solution to be built into y-prosemirror.

export class IndestructibleUndoManager extends Y.UndoManager {
	constructor(type, opts) {
		super(type, opts);
	}

	destroy(actuallyDestroy = false) {
		if (actuallyDestroy) super.destroy();
		// y-prosemirror call does not end up destroying
	}
}