search

yjs / y-prosemirror

ProseMirror editor binding for Yjs
https://demos.yjs.dev/prosemirror/prosemirror.html
MIT License
335 stars 116 forks source link

fix: abort scheduled processes on destruction #71

Closed hoebbelsB closed 2 years ago

hoebbelsB commented 2 years ago

fixes #70

ValentaTomas commented 2 years ago

This fix (cursor-plugin.js part specifically) solved my problem with #70 too - it is an almost constant problem when you allow user to switch between several ProseMirror editors.

Are there some other problems with this PR request that prevents it from being merged?

ValentaTomas commented 2 years ago

Shouldn't we also clear setTimeout from https://github.com/yjs/y-prosemirror/blob/f593d68a90ec996169718ac33b4c9d35f977ee42/src/plugins/sync-plugin.js#L114 ?

Also how about using timeout from lib0/eventloop-- the timeout is already used in this package when updating cursor.

I would gladly make the changes, but I don't want to create a new PR that is essentially the same as this one.

@dmonad Can you please check this issue? I'm using these fixed in our project, but I would be really glad if I could use the y-prosemirror official package again.

tommoor commented 2 years ago

This seems like a clean bugfix~ not sure why it's sat for months.

dmonad commented 2 years ago

Thank you for the PR @hoebbelsB and sorry that it took so long.

I went for a different approach that checks whether the binding is destroyed. I prefer not to clear timeouts because that leads to further issues in the future. setMeta, for example is used in third-party packages as well that don't know that they should clear the timeout.

@ValentaTomas I implemented your suggestion as well and switched from setTimeout to eventloop.timeout. Thanks for the suggestion

ValentaTomas commented 2 years ago

Thank you for your work!