-
Notifications
You must be signed in to change notification settings - Fork 2k
[WIP][lexical-yjs][lexical-react] Feature: initial implementation of collab v2 #7616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
); | ||
} | ||
|
||
export function useYjsCollaborationV2__EXPERIMENTAL( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed shouldBootstrap
from v2 because it's a big foot-gun in v1. The consistent recommendation in Discord is to set it to false, so figured may as well use this opportunity to remove it entirely.
syncCursorPositionsFn
is tied specifically to Binding
. I haven't looked into cursor syncing for the rewrite yet, when I get to that, I'll try and make the function generic.
fbfd4f5
to
70e1f5a
Compare
…nternal with shared v1/v2 logic
486f58e
to
8d91369
Compare
@@ -38,7 +37,7 @@ test.describe('Collaboration', () => { | |||
isCollab, | |||
browserName, | |||
}) => { | |||
test.skip(!isCollab || IS_MAC); | |||
test.skip(!isCollab); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed because v2 doesn't support shouldBootstrap
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per above, this is basically the same as createNodeFromYElement, except that it takes dirtyElements
and updates existing nodes.
): Y.XmlText => { | ||
const type = new Y.XmlText(); | ||
const delta = nodes.map((node) => ({ | ||
attributes: {__properties: propertiesToAttributes(node, meta)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where dirtyElements
is used to update YFragments without removing them from the mapping.
@@ -33,6 +33,7 @@ export const DEFAULT_SETTINGS = { | |||
tableCellBackgroundColor: true, | |||
tableCellMerge: true, | |||
tableHorizontalScroll: true, | |||
useCollabV2: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding as a setting but not exposing it through the settings menu yet, given how early days this still is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't been able to do invest the time to do a very thorough review, but the things that caught my eye were mostly just the sections where this is handling exceptions indiscriminately
return { | ||
...createBaseBinding(editor, id, doc, docMap, excludedProperties), | ||
mapping: new Map(), | ||
root: doc.get('root-v2', XmlElement) as XmlElement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this has a different default to avoid bad behavior when different versions of collab are used with the same doc, although maybe it should be parameterized further in some way to also solve the use case from #6483?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do.
} catch (e) { | ||
// an error occured while creating the node. This is probably a result of a concurrent action. | ||
// TODO(collab-v2): also delete the mapped node from editor state. | ||
el.doc!.transact((transaction) => { | ||
el._item!.delete(transaction); | ||
}, meta); | ||
meta.mapping.delete(el); | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to narrow this down to specific exceptions we'd expect to see and/or to a smaller portion of that code. A whole lot is happening in that block and this would make it quite hard to debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd been meaning to come back to this. Given it's in the functions for going from Yjs->Lexical, I'd rather just throw all errors and let the Lexical error boundary handle it. Just deleting data from Yjs seems needlessly dangerous. (It's only like this now because that's how y-prosemirror works.)
} catch (e) { | ||
// an error occured while creating the node. This is probably a result of a concurrent action. | ||
// TODO(collab-v2): also delete the mapped text nodes from editor state. | ||
text.doc!.transact((transaction) => { | ||
text._item!.delete(transaction); | ||
}); | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, I think this will make debugging very difficult
Description
Introduces a new
useYjsCollaborationV2__EXPERIMENTAL
hook andCollaborationPluginV2__EXPERIMENTAL
plugin. 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
syncLexicalUpdateToYjs
andsyncYjsChangesToLexical
. 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.each
and passinguseCollabV2
down into the relevant functions (createTestConnection
andexpectCorrectInitialContent
).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.