yjs / y-prosemirror

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

y-prosemirror fires two updates for each change #121

Closed ocavue closed 1 year ago

ocavue commented 2 years ago

Please save me some time and use the following template. In 90% of all issues I can't reproduce the problem because I don't know what exactly you are doing, in which environment, or which y-* version is responsible. Just use the following template even if you think the problem is obvious.

Checklist

Describe the bug

After updating y-prosemirror from 1.0.20 to 1.1.0, every keypress will trigger the update method for every plugin twice. This causes a lot of issues if a plugin want to compare the current state with the prev state.

To Reproduce

I wrote a minimal reproduction to show this issue. In this example, I have a ProseMirror plugin to show whether or not the cursor is changed in the last operation. The code of this plugin is in prosemirror.ts.

  1. Open https://stackblitz.com/github/ocavue/y-prosemirror-playground/tree/issue-double-update-1.0.20 and https://stackblitz.com/github/ocavue/y-prosemirror-playground/tree/issue-double-update-1.1.2. They have the same code but with different versions of y-prosemirror
  2. Click the Disconnect button. We don't want them to affect each other.
  3. Typing something.
  4. In the 1.0.20 window, you will see Is your cursor changed? true. While in the 1.1.2 window, you will see false.
  5. Open the console. Every keypress will trigger one plugin update in 1.0.20, but two updates in 1.1.2.

Expected behavior

Every keypress will only trigger one plugin update, as what we used to have in y-prosemirror 1.0.20

Screenshots

https://share.cleanshot.com/NJlBDh

Additional context

In commit 2d06ece86f8359b14e9f6b27bb5746e2ca2efc82, pluginState.doc.transact was added inside syncPlugin's update function. Calling pluginState.doc.transact will trigger _typeChanged because it has been observed. tr.replace will be called in _typeChanged, which causes the second update.

ocavue commented 2 years ago

This issue should share the same underlying reason as https://github.com/yjs/y-prosemirror/issues/113

jamesopti commented 2 years ago

+1 - It appears now that due to the same commit, NodeSelections are converted to TextSelections anytime the document changes, as the _typeChanged mux function ends up getting called and actually replaces the entire document (like a remote change).

jamesopti commented 2 years ago

Any updates on this one?

maccman commented 2 years ago

This one seems quite important @dmonad - and I think @ocavue has a PR that fixes it.

dmonad commented 1 year ago

Fixed and released :+1:

maccman commented 1 year ago

Amazing! Thank you.