BitPhinix / slate-yjs

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

Path doesn't match yText, yTarget spans multiple nodes on 'false' attribute values #417

Closed suarezanton closed 8 months ago

suarezanton commented 12 months ago

Hello,

First thanks for the great library!

I've seen some issues logged for this particular error, but none that exactly match what I'm experiencing. We are getting a bunch of these errors in our editor:

location.ts:43 Uncaught (in promise) Error: Path doesn't match yText, yTarget spans multiple nodes
    at getYTarget (location.ts:43:11)
    at getYTarget (location.ts:54:12)
    at insertText (insertText.ts:11:42)
    at applySlateOp (index.ts:27:3)
    at withYjs.ts:260:11
    at Array.forEach (<anonymous>)
    at withYjs.ts:258:17
    at transact (Transaction.js:425:14)
    at Doc.transact (Doc.js:167:12)
    at withYjs.ts:257:24

I tracked most of these occurrences down to how we were toggling addMark / removeMark on the slate editor. tl;dr we were not using removeMark on slate editor, but instead saving a 'false' value instead. So for example if toggling bold text on / off, we'd end up with { bold: true, text: 'foo' } or { bold: false, text: 'foo' } on the node rather than flat out removing the bold attribute. We've since changed the logic around so we use removeMark and so hopefully will start to see the number of these errors decrease.

We do still have historical documents though that would have these falsey values saved on the nodes. So I don't foresee this error going away completely.

Is there anything you would suggest here, or has anyone run into something similar?

Digging around in the code I'm not actually sure if this behavior is caused by yjs when it creates the delta, or if it's slate-yjs in normalizeInsertDelta (specifically when evaluating merge value).

Here is a video clip of the problem happening in the sandbox environment with an initialValue.json of:

[
  {
    "type": "paragraph",
    "children": [
      { "bold": false, "text": "This text has a bold = false attribute" }
    ]
  }
]

https://github.com/BitPhinix/slate-yjs/assets/17148603/a7400131-9f9c-43bc-ae74-3d2e33691b4e

Last I also hijacked one of the tests in the repo to demonstrate how those nodes are being split up with a false attribute value in case that is helpful as well (this test will fail):

/** @jsx jsx */
import { Editor } from 'slate';
import { jsx } from '../../../../../support/jsx';

export const input = (
  <editor>
    <unstyled>
      <text bold={false}>
        Hello <cursor />!
      </text>
    </unstyled>
  </editor>
);

export const expected = (
  <editor>
    <unstyled>
      <text bold={false}>
        Hello world
        <cursor />!
      </text>
    </unstyled>
  </editor>
);

export function run(editor: Editor) {
  editor.insertText('world');
}

Thanks much for any help! Happy to work on a PR for this issue if it's something that could be fixed here.

jul13579 commented 8 months ago

Hi @suarezanton!

I'm currently facing the same error message, however with different falsy values. In my case I got attributes that are set to empty strings. When debugging the problem, I came to the conclusion that this problem might better be reported to Yjs itself, because of this line of code I stumbled over. Yjs by default seems to unset any falsy attributes when inserting text into the YText, probably in favor of storage savings. In return, slate-yjs isn't able to merge the deltas into a single delta during normalizeInsertDelta, which is called from getYText -> yTextToInsertDelta. getYText however expects a single delta when hitting a slate leaf node. Which makes sense, since a leaf node either has some attributes set, or it hasn't - not a mix of both (as the deltas suggest).

I doubt that Yjs will approve adjustments that allow falsy attribute values - because of the storage savings assumption. Therefore I think we will have to adapt to the Yjs way of doing things, and make sure we remove falsy attributes from the YDoc alltogether. In my case this should be possible and actually is desirable. To make sure the Yjs implementation really isn't flawed, I opened a PR that's currently awaiting feedback.

jul13579 commented 8 months ago

Wow, that was quick: https://github.com/yjs/yjs/pull/619#issuecomment-1972921346

suarezanton commented 8 months ago

Hey @jul13579 thanks so much for following up on this! It's been less of an issue since I mentioned we basically changed the way our slate plugins were toggling those values on / off. But given the large amount of historical data we have we still run into this when older documents are hydrated for the first time into the yjs realm.

I'll pull down that new yjs release and give it a go on my end. Assuming that works and it does what I assume it will I will close this issue out. Thanks again!

suarezanton commented 8 months ago

As expected yjs 13.6.14 fixes this issue described here. Closing this issue.

BitPhinix commented 8 months ago

Thanks for tracking this down!