-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Real-time collaboration: Fix persisted document meta and add tests #73985
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
Conversation
packages/sync/src/utils.ts
Outdated
|
|
||
| export function createYjsDoc( documentMeta: Record< string, unknown > ): Y.Doc { | ||
| interface DocumentMeta { | ||
| [ key: string ]: boolean | number | string; |
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'm kind of hesitant here, are these the only meta attributes that third-party blocks can have, I wonder if it's possible to have arrays and nested objects too.
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.
@youknowriad CRDT document metadata are in-memory metadata of the CRDT document instance (example: a boolean indicating whether the document was loaded from persistence / post meta). The sync package fully controls this metadata and it is not persisted or synced. I've added a comment in 2beb2f4 that hopefully makes this more clear.
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.
Ah I thought this was about post meta
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 2beb2f4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20241061840
|
| type DocumentMeta = Record< string, DocumentMetaValue >; | ||
| type DocumentMetaValue = boolean | number | string; | ||
|
|
||
| export function createYjsDoc( documentMeta: DocumentMeta = {} ): Y.Doc { |
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.
Why are we messing with these "meta"? Is it because we need to persist the doc in the database or something, so we need some serialized information.
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.
The main use case for CRDT document metadata is local state that is colocated with the document. "Local" because we don't want this metadata to be synced to peers or persisted—it applies only to this document instance, in-memory.
We have one use case currently: We use CRDT document metadata to store a marker that the CRDT document was loaded from persistence. This allows us to have code in core-data that runs only when we are validating persisted CRDT documents:
gutenberg/packages/core-data/src/utils/crdt.ts
Lines 282 to 292 in dbcbe3d
| if ( | |
| ydoc.meta?.get( CRDT_DOC_META_PERSISTENCE_KEY ) && | |
| editedRecord.content | |
| ) { | |
| const blocks = ymap.get( 'blocks' ) as YBlocks; | |
| return ( | |
| __unstableSerializeAndClean( | |
| blocks.toJSON() | |
| ).trim() !== editedRecord.content.raw.trim() | |
| ); | |
| } |
tl;dr: They are in-memory descriptors for a CRDT document.
What?
Improve the format and type of persisted CRDT document meta.
Why?
The
unknownrecord value type allowed consuming code to incorrectly nest document meta in ametaproperty. As a result, persisted CRDT documents could not be correctly validated, resulting in incorrect merge behavior and CRDT document invalidation.How?
Provide the CRDT document meta correctly, improve the type to prevent nested meta in the future, and add tests.