Open ascott18 opened 2 years ago
This looks like a duplicate of https://github.com/yjs/y-prosemirror/issues/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.
@dmonad Yes, it looks like the same issue.
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
]
}
})
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:
what do you think? @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.
This PR https://github.com/yjs/yjs/pull/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: https://github.com/hamflx/y-prosemirror/commit/fd79c58145e6cad99dddd8d7a2c9a4d7f582d8dc
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)
preventDestroy
and delayedDestroy
methods, so nothing happens.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)
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.
If there is no better way, is it possible to consider my proposal?
@dmonad
@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.
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
}
}
Checklist
Describe the bug Scenario is as follows:
yUndoPlugin()
(default settings)UndoManager
is subsequently setup withafterTransaction
listener on the YDoc. Everything normal up to this point.EditorView
has been initialized.Expected behavior The
UndoManager
should not be destroyed when theEditorView
is destroyed, since theUndoManager
is initialized by the plugin, not by the plugin'sEditorView
. The plugin itself and the plugin's EditorView have different lifecycles.Initializing the
UndoManager
along with theEditorView
(e.g. inview: 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 theEditorView
of undo-plugin. Just leave the lifecycle of theUndoManager
tied to the lifecycle of the YDoc (which it already hooks into in UndoManager's ctor).Environment Information