[undo-plugin] UndoManager is destroyed on view.destroy but created in state.init
jamesopti opened this issue · 8 comments
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
When Prosemirror view calls EditorView.prototype.updatePluginViews, it will destroy existing plugin views and recreate them if a new plugin is added.
This is an issue if the yUndoPlugin has already been registered because it creates the instance of YJS UndoManager in the state lifecycle but destroys it with the view lifecycle.
To Reproduce
Steps to reproduce the behavior:
- Load a prosemirror editor with the y-sync and y-undo plugins enabled
- Register a new prosemirror plugin
- Observe the UndoManager get destroyed
- The UndoManager remains destroyed
Expected behavior
A clear and concise description of what you expected to happen.
Screenshots
If applicable, add screenshots to help explain your problem.
Environment Information
- Chrome Version 99.0.4844.84 (Official Build) (arm64)
➜ client git:(main) ✗ yarn info y-prosemirror
└─ y-prosemirror@npm:1.0.17
├─ Instances: 1
├─ Version: 1.0.17
│
└─ Dependencies
└─ lib0@npm:^0.2.42 → npm:0.2.48
➜ client git:(main) ✗ yarn info yjs
└─ yjs@npm:13.5.33
├─ Version: 13.5.33
│
└─ Dependencies
└─ lib0@npm:^0.2.48 → npm:0.2.48
Additional context
This is a new problem with the introduction of the .destroy method in yjs/yjs@e9a0dc4
The introduction of destroy on the undo manager was necessary because these undo managers were never cleanup up after the editor was destroyed. Hence, there was still one Y.UndoManager active for each binding ever created, tracking operations that would never be undone. We even had multiple undo managers for the same document.
You are right though, the current implementation is also not ideal as it deletes the undo manager when the view is destroyed.
Is there some kind of method to recognize when the editor has been destroyed, or when the Y.UndoManager state was unregistered?
Is there some kind of method to recognize when the editor has been destroyed, or when the Y.UndoManager state was unregistered?
This is a great question. I'm not sure off the top of my head and after looking through the code. I'd have to ask @philippkuehn or @marijnh about it.
The only idea that comes to mind here is to do lazy instantiation of the UndoManager instance in the .apply method. I.e. each time apply runs, check if the current instance has been destroyed and if so, create a new one as part of the apply lifecycle. It might be more proper to even remove the UndoManager from the plugin state in view.destroy via a transaction.
In most applications, I think plugin registration only happens on load, so this shouldn't really fire too often.
Is there some kind of method to recognize when the editor has been destroyed
Plugin views' destroy method should work for that.
Thank you for jumping in @marijnh! I'm currently using the Plugin view's destroy method to destroy the UndoManager. However, according to @jamesopti the destroy method is also fired when the plugins are reconfigured. The problem is that we don't want to destroy the stateful UndoManager in this case because we would lose all the captured undo-history.
The stateful UndoManager currently lives on a ProseMirror state object. I realize that this is not ideal. I chose this approach because I need to access the UndoManager from other plugins. y-codemirror.next stores the UndoManager on an Annotation instead, which seems to be much better suited for this use case.
@jamesopti I think there is no ideal solution for this problem. I suggest that we keep the current approach to destroy the undo manager in destroy. However, we can recreate the undo manager when the undo manager is still used after the view has been destroyed.
Furthermore, we can enforce that the undo manager is not going to be destroyed when it is submitted by you manually. This gives you the control to reconfigure plugins without destroying the undo manager. However, this means that you are responsible for destroying the undo manager when you don't need it anymore.
Is this feature still needed or did you find another solution @jamesopti ?
I'm closing for now, but feel free to reopen.
@jamesopti @dmonad could we get this reopened? At the moment registering a Prosemirror plugin after the y-undo plugin breaks all undo functionality. This is because registering a plugin destroys previous plugin views which destroys the undo manager which causes it to stop listening to the afterTransaction event. From my understanding it never starts listening to this event again when the view is recreated.
If we can get it to start listening again when the view is created, that could be a quick fix for now. Thoughts?
Update: actually perhaps it's just registering a plugin through Tiptap which is causing this.