BitPhinix / slate-yjs

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

[Feature Request]: Sync empty text nodes #343

Open productivityindustries opened 2 years ago

productivityindustries commented 2 years ago

Hi, I wrote a simple unit test to validate the conversion from Slate nodes to Yjs shared objects and reverse and, to my surprise, it fails.

import { slateNodesToInsertDelta, yTextToSlateElement } from '@slate-yjs/core';
import { expect } from 'chai';
import { Node } from 'slate';
import * as Y from 'yjs';

describe('slate', () => {
  it('should correctly convert Slate nodes', () => {
    const nodes = [
      {
        children: [
          {
            text: 'Test:',
          },
        ],
        type: 'paragraph',
        id: 'ed99db3b-9dc4-47ad-b8b7-4bfb44e3cd1e',
      },
      {
        children: [
          {
            text: '',
          },
          {
            children: [
              {
                text: '',
                hyperlink: {
                  url: 'https://www.example.com',
                },
                underlined: true,
                fontColor: '#0000ff',
              },
            ],
            type: 'inline-image',
            transform: {
              angle: 0,
              width: '134px',
              height: '50px',
            },
            blobID: '2694b3a2-afae-4fcc-8a60-44334bc3a04b',
            blobURL: 'https://www.example.com/image.png',
            blobFormats: ['original', 'dataUrl'],
          },
          {
            text: '',
          },
        ],
        type: 'paragraph',
        id: 'ca804088-30ff-4f51-b8ea-4f99c846fa0e',
      },
      {
        children: [
          {
            text: '',
          },
        ],
        type: 'paragraph',
        id: '53bf071f-4929-4c8c-94a4-cebd33c48ddc',
      },
      {
        children: [
          {
            text: '',
          },
        ],
        type: 'paragraph',
        id: '9326fbf8-f41c-44c3-9954-c06b0ccfbc30',
      },
    ];
    const ydoc = new Y.Doc();
    const delta = slateNodesToInsertDelta(nodes);
    const xmlText = ydoc.get('test', Y.XmlText) as Y.XmlText;
    xmlText.applyDelta(delta, { sanitize: false });
    const node = yTextToSlateElement(xmlText);
    expect(node.children).to.deep.equal(nodes);
  });
});

Test output:

  slate
    1) should correctly convert Slate nodes

  0 passing (31ms)
  1 failing

  1) slate
       should correctly convert Slate nodes:

      AssertionError: expected [ { type: 'paragraph', …(2) }, …(3) ] to deeply equal [ { …(3) }, …(3) ]
      + expected - actual

         }
         {
           "children": [
             {
      +        "text": ""
      +      }
      +      {
               "blobFormats": [
                 "original"
                 "dataUrl"
               ]
               "blobID": "2694b3a2-afae-4fcc-8a60-44334bc3a04b"
               "blobURL": "https://www.example.com/image.png"
               "children": [
                 {
      +            "fontColor": "#0000ff"
      +            "hyperlink": {
      +              "url": "https://www.example.com"
      +            }
                   "text": ""
      +            "underlined": true
                 }
               ]
               "transform": {
                 "angle": 0
--
                 "width": "134px"
               }
               "type": "inline-image"
             }
      +      {
      +        "text": ""
      +      }
           ]
           "id": "ca804088-30ff-4f51-b8ea-4f99c846fa0e"
           "type": "paragraph"
         }

I took as an example a document created in our webapp with an inline image (Slate automatically surrounds inline elements with empty texts) with an hyperlink attached (an empty text with attributes is perfectly valid in Slate). Digging into the code, I realized the issue root cause is in the Yjs business logic skipping any insertion of empty strings. This misalignment breaks synchronization between clients, and also the storing of document data on the server side. The bridge between Slate and Yjs should be able to fully reconstruct the original node structure, as far as it complies with the Slate model.

BitPhinix commented 2 years ago

Hi @productivityindustries,

This isn't a bug - it's an intentional tradeoff of the representation of the slate document inside yjs. Slate-yjs relies on slates default normalization rules to be run after applying a remote change, which will add the empty text elements back in, causing the remote and local state to match (except for formatting attributes on blank text nodes). Appling changes with mismatches in the empty text nodes won't cause issues.

While we could sync the formatting attributes of empty text nodes using yjs embeds, I intentionally decided not to support it for now, as slate treats them as disposable anyway (e.g., it will merge them, ignoring the formatting attributes). So storing any critical information on them should be avoided. Instead, you could probably add back the URL attribute using a normalization rule in your example, although it's hard to say without knowing the specifics.

eagowang commented 1 year ago

Hi @productivityindustries, I have the same problem, because the empty text not sync, the doc init but link(inline element) not surround with empty text node after refresh browser. Did you solve the problem?

eagowang commented 1 year ago

Hi @productivityindustries, I have the same problem, because the empty text not sync, the doc init but link(inline element) not surround with empty text node after refresh browser. Did you solve the problem?

Hi @productivityindustries @BitPhinix, Call Editor.normalize(editor, {force: true}) when first applyRemoteEvents can solve this problem, but I don't know if there other problems