-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Collab editing: ensure block attributes are serializable #57025
Conversation
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
Size Change: +370 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in d042da7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7201812580
|
packages/core-data/src/entities.js
Outdated
@@ -317,11 +339,15 @@ async function loadPostTypeEntities() { | |||
}, | |||
applyChangesToDoc: ( doc, changes ) => { | |||
const document = doc.getMap( 'document' ); | |||
|
|||
Object.entries( changes ).forEach( ( [ key, value ] ) => { | |||
if ( | |||
document.get( key ) !== value && |
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 problem is that now, the document will always be different for the "blocks" property. Meaning if a user changes the "title", the "blocks" will also be considered "modified" and it will send them over the wire unnecessarily. I think there's probably several small consequences for this.
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.
Because it's a new array? What if we memo it? Ensure that for the same array, we return the same "serialisable" array.
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 requirement in CRDT "maps" is to avoid calling set
if there's no change. In this case, if the "blocks" object is still the same that exists in the document
, we should not call set
.
For now it's not sufficient to memoize just the attribute because it will create a new "blocks" property anyway so we'll endup calling the set
function.
That said, right now we're not "decomposing" the blocks property in CRDT which we should be doing to allow parallel modification of blocks and merging the changes. And once we do that, it will actually become similar to "memoize" per attribute.
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 requirement in CRDT "maps" is to avoid calling
set
if there's no change. In this case, if the "blocks" object is still the same that exists in thedocument
, we should not callset
.
But we're not checking right now if the blocks have changed?
I'm not sure I understand much of the rest, but I want to understand if there's going to be a fundamental problem with it. 😅 The last bit of your comment sounds like it may be ok.
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.
But we're not checking right now if the blocks have changed?
we are here; document.get( key ) !== value
in this line if the "blocks" key in the document contains the same value as the one that we receive from state, we don't call the set
function.
With the changes in this PR, the "blocks" key in the document will always be different because it's created every time when setting it, so there's no way it could match the value we get from the state.
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.
Oh missed that sorry. I'll try to fix it.
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 should work, right? Or am I misunderstanding it? d042da7
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.
LGTM 👍
What?
I'm not sure if there's a better way to do this with Yjs directly. This PR converts all rich text instances in block attributes to primitive values (strings) before handing it over to Yjs.
Why?
Currently syncing of rich text attributes is broken.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast