[lexical-yjs][lexical-react] Feature: initial implementation of collab v2#7616
[lexical-yjs][lexical-react] Feature: initial implementation of collab v2#7616etrepum merged 30 commits intofacebook:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
facebook-github-bot
added
the
CLA Signed
james-atticus
force-pushed
the
yjs-v2-boilerplate
branch
from
fbfd4f5 to
70e1f5a
Compare
james-atticus
requested review from
Fetz,
Sahejkm,
acywatson,
fantactuka,
ivailop7,
lilshady,
potatowagon,
takuyakanbr and
zurfyx
as code owners
james-atticus
changed the title
james-atticus
changed the title
…nternal with shared v1/v2 logic
james-atticus
force-pushed
the
yjs-v2-boilerplate
branch
from
486f58e to
8d91369
Compare
| browserName, | ||
| }) => { | ||
| test.skip(!isCollab || IS_MAC); | ||
| test.skip(!isCollab); |
There was a problem hiding this comment.
Drive-by: this test works fine on mac
| if (useCollabV2) { | ||
| // Manually bootstrap editor state. | ||
| await waitForReact(() => { | ||
| client1.update(() => $getRoot().append($createParagraphNode())); |
There was a problem hiding this comment.
Needed because v2 doesn't support shouldBootstrap
packages/lexical-yjs/src/Bindings.ts
Outdated
| export type LexicalMapping = Map< | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| YAbstractType<any>, | ||
| // Either a node if type is YXmlElement or an Array of text nodes if YXmlText | ||
| LexicalNode | Array<TextNode> | ||
| >; |
There was a problem hiding this comment.
Equivalent of the type in y-prosemirror.
| const dirtyElements = new Set<NodeKey>(); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const collectDirty = (_value: any, type: YAbstractType<any>) => { | ||
| if (binding.mapping.has(type)) { | ||
| const node = binding.mapping.get(type)!; | ||
| if (!(node instanceof Array)) { | ||
| dirtyElements.add(node.getKey()); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
In y-prosemirror, for any YType that's been modified, the corresponding entry is deleted from the mapping. This then causes the sync code to recreate the code in createNodeIfNotExists. In Lexical however, we don't want to do this - we want to update the existing node if possible (i.e. keep the same nodeKey).
| /** | ||
| * @return Returns node if node could be created. Otherwise it deletes the yjs type and returns null | ||
| */ | ||
| export const $createOrUpdateNodeFromYElement = ( |
There was a problem hiding this comment.
Per above, this is basically the same as createNodeFromYElement, except that it takes dirtyElements and updates existing nodes.
packages/lexical-yjs/src/SyncV2.ts
Outdated
| ): Y.XmlText => { | ||
| const type = new Y.XmlText(); | ||
| const delta = nodes.map((node) => ({ | ||
| attributes: {__properties: propertiesToAttributes(node, meta)}, |
There was a problem hiding this comment.
y-prosemirror has marks (marksToAttributes(node.marks, meta)) for text nodes and attributes (node.attrs) for element nodes. Lexical does have this same delineation - just got the one propertiesToAttributes helper function.
packages/lexical-yjs/src/SyncV2.ts
Outdated
| const rightY = yChildren[yChildCnt - right - 1]; | ||
| const rightP = pChildren[pChildCnt - right - 1]; | ||
| if (mappedIdentity(meta.mapping.get(rightY), rightP)) { | ||
| if (rightP instanceof ElementNode && dirtyElements.has(rightP.getKey())) { |
There was a problem hiding this comment.
This is where dirtyElements is used to update YFragments without removing them from the mapping.
| tableCellBackgroundColor: true, | ||
| tableCellMerge: true, | ||
| tableHorizontalScroll: true, | ||
| useCollabV2: false, |
There was a problem hiding this comment.
Adding as a setting but not exposing it through the settings menu yet, given how early days this still is.
james-atticus
force-pushed
the
yjs-v2-boilerplate
branch
from
eb45154 to
a9d7f44
Compare
etrepum
left a comment
etrepum
left a comment
There was a problem hiding this comment.
I think this is in a good state, it certainly seems to be an improvement over v1. Since it's experimental and not a default I think it's fairly low risk?
It's a shame that NodeState has to be serialized to/from string for Text nodes but if that becomes a bottleneck I'm sure that's something we could deal with later one way or another.
3 participants
Description
Introduces a new
useYjsCollaborationV2__EXPERIMENTALhook andCollaborationPluginV2__EXPERIMENTALplugin. Implementation of binding is heavily based off y-prosemirror. Tree shaking should mean that consumers who only use one version won't have a noticeable increase in bundle size.The first commit is mostly refactoring, splitting out some shared logic from the original hook into
useYjsCollaborationInternal(open to naming suggestions).The second commit adds v2 implementations of
syncLexicalUpdateToYjsandsyncYjsChangesToLexical. Notably missing is all logic related to keeping selection updated and syncing cursors. That will come in a later PR.Test plan
Unit tests have been updated to run with both v1 and v2 collab code. Despite the large diff (Github seems to be struggling even with whitespace hidden), the only change was to add the wrapping in
describe.eachand passinguseCollabV2down into the relevant functions (createTestConnectionandexpectCorrectInitialContent).I have E2E tests running with v2 locally, however most of them fail because the selection in the right viewer isn't updated, so assertions about things like toolbar state fail. I'll look at enabling collab v2 in tests in a later PR.