BitPhinix / slate-yjs

Yjs binding for Slate
https://docs.slate-yjs.dev
MIT License
508 stars 72 forks source link

Awareness Update before State Update #374

Closed superBertBerg closed 1 year ago

superBertBerg commented 1 year ago

Hey @BitPhinix thanks for that decent work!

I'm hunting a bug since hour's. Correct me if i'm wrong there is no guarantee that an awareness update comes before it's state change, especially over the wire? So that https://github.com/BitPhinix/slate-yjs/blob/d897d5d0995b3390229228ef7609091622e2344a/packages/react/src/hooks/useRemoteCursorOverlayPositions.tsx#L106-L110 can crash not finding a position?

superBertBerg commented 1 year ago

Or even if that is guaranteed it's possible that slate is not already finished rendering that state?

BitPhinix commented 1 year ago

Hmm, I've been thinking about this one and it shouldn't be happening. Awareness updates definitely shouldn't arrive before their corresponding state update, but even if that happens it wouldn't cause an issue within slate-yjs itself because of the way relative positions work.

So if there is a race-condition it's the cursor change causing a render before slate-react re-renders which possibly could be happening. But to investigate that it would be immensely helpful to have something where I could reproduce the issue. Could you get it to happen on slate-yjs.dev?

superBertBerg commented 1 year ago

i'll provide a minimal reproduction asap and provide a repo

FindAPattern commented 1 year ago

I've also been dealing with this exact issue for the past couple days. It only occurs if you start typing at the very end of the document, since the cursor range will be one character past the range in Slate.

Thus far I've figured out that the selection range is being calculated before the DOM updates, but after editor.children has been updated. It compares the offset to text nodes within the DOM, which causes it to throw an error. I suspect it's user error, since I'm using this exact code in the same application in a different component, but that one isn't running into this issue.

Edit: I think this has to do with React 18's automatic batching, not out-of-order awareness / state updates.

Posted a bit more detail (and a potential workaround) in another issue: https://github.com/BitPhinix/slate-yjs/issues/372#issuecomment-1322817249

superBertBerg commented 1 year ago

I can confirm that, switching back to the old render method solved it for me which should deactivate that kind of batching.

import { render } from 'react-dom';

@FindAPattern Thanks for that, that bug drove me mad especially because I searched in the wrong direction. That answer is relief!

@BitPhinix I did a bunch of tests trying to get a race condition but could not. My bug is pretty sure the bug mentioned in #372 so i'm closing this