Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yjs extension leaks yjs UndoManager instances when editor re-renders #1448

Closed
ankon opened this issue Dec 29, 2021 · 2 comments · Fixed by #1457
Closed

Yjs extension leaks yjs UndoManager instances when editor re-renders #1448

ankon opened this issue Dec 29, 2021 · 2 comments · Fixed by #1457
Labels
type: bug 🪲 Something isn't working

Comments

@ankon
Copy link
Contributor

ankon commented Dec 29, 2021

Summary

Yjs extension leaks yjs UndoManager instances when the editor re-renders:

  1. The yjs extension creates a yUndoPlugin instance as part of its "external" plugins (
    /**
    * Create the yjs plugins.
    */
    createExternalPlugins(): ProsemirrorPlugin[] {
    const {
    syncPluginOptions,
    cursorBuilder,
    getSelection,
    cursorStateField,
    protectedNodes,
    trackedOrigins,
    } = this.options;
    const yDoc = this.provider.doc;
    const type = yDoc.getXmlFragment('prosemirror');
    return [
    ySyncPlugin(type, syncPluginOptions),
    yCursorPlugin(
    this.provider.awareness,
    { cursorBuilder, cursorStateField, getSelection },
    cursorStateField,
    ),
    yUndoPlugin({ protectedNodes, trackedOrigins }),
    ];
    }
    )
  2. The yUndoPlugin creates a new Yjs UndoManager instance in it's initState callback (https://github.com/yjs/y-prosemirror/blob/039be96fb520e972fa37f0e896b585ef19da1beb/src/plugins/undo-plugin.js#L35-L48)
  3. The UndoManager attaches itself to the document's afterTransaction event (https://github.com/yjs/yjs/blob/645f05b0bb2435552582f587f5379c33adda6ffe/src/utils/UndoManager.js#L166)
  4. Remirror creates a new state on each rerender as part of the useReactFramework hook:
    const initialEditorState = state
    ? state
    : manager.createState({ content: initialContent, selection: initialSelection });

Steps to reproduce

sandbox TBD, basically:

  1. Configure remirror with the yjs extension
  2. Trigger rerenders of the editor (for example through hook changes etc)

Expected results

Everything is happy.

Actual results

  • Y.Doc instance slowly collects more afterTransaction handlers
  • Editor becomes slower and slower after key presses as the browser is busy creating undo transactions over and over
  • Eventually browser crashes due to out-of-memory

Possible Solution

y-prosemirror allows to provide a undo manager. We have forked the remirror yjs extension again to do that, see Collaborne/remirror-extension-yjs@7c77917 for the commit.
In remirror "proper" you will have to update the y-prosemirror dependency to at least 1.0.14 to include yjs/y-prosemirror#87, due to historic reasons we use our own y-prosemirror and basically merged that PR.

@ankon ankon added the type: bug 🪲 Something isn't working label Dec 29, 2021
@ocavue
Copy link
Member

ocavue commented Jan 4, 2022

I don't know much about YJS, but I'd like to merge it if you can submit a PR to Remirror. (I can take care of the updating y-prosemirror dependency)

@ankon
Copy link
Contributor Author

ankon commented Jan 4, 2022

See #1457 for a proposed fix (basically a merge of our changes + two tests)

ankon added a commit to ankon/remirror that referenced this issue Jan 4, 2022
ankon added a commit to ankon/remirror that referenced this issue Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🪲 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants