Skip to content
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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Collab editing: ensure block attributes are serializable #57025

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0cc253
Collab editing: ensure block attributes are serialisable
ellatrix Dec 13, 2023
d042da7
Only set document if changed
ellatrix Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions packages/core-data/src/entities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { capitalCase, pascalCase } from 'change-case';
*/
import apiFetch from '@wordpress/api-fetch';
import { __ } from '@wordpress/i18n';
import { RichTextData } from '@wordpress/rich-text';

/**
* Internal dependencies
Expand Down Expand Up @@ -275,6 +276,27 @@ export const prePersistPostType = ( persistedRecord, edits ) => {
return newEdits;
};

function makeBlockAttributesSerializable( attributes ) {
const newAttributes = { ...attributes };
for ( const [ key, value ] of Object.entries( attributes ) ) {
if ( value instanceof RichTextData ) {
newAttributes[ key ] = value.valueOf();
}
}
return newAttributes;
}

function makeBlocksSerializable( blocks ) {
return blocks.map( ( block ) => {
const { innerBlocks, attributes, ...rest } = block;
return {
...rest,
attributes: makeBlockAttributesSerializable( attributes ),
innerBlocks: makeBlocksSerializable( innerBlocks ),
};
} );
}

/**
* Returns the list of post type entities.
*
Expand Down Expand Up @@ -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 &&
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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

typeof value !== 'function'
) {
if ( key === 'blocks' ) {
value = makeBlocksSerializable( value );
}
document.set( key, value );
}
} );
Expand Down
Loading