-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Real-time collaboration: Add more robust mergeCrdtBlocks() testing #74304
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
Real-time collaboration: Add more robust mergeCrdtBlocks() testing #74304
Conversation
|
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. |
|
@chriszarate Added a few new tests for |
| if ( ! clientId ) { | ||
| continue; | ||
| if ( clientId ) { |
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.
is this logic inverted for any particular reason?
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 just allows the recursion on innerBlocks to run even if the current block is missing a client ID. I'm not clear on when a clientId will be missing from a block or if that's even a real possibility, but it was accounted for in the original code. Do you have any insight on when this happens? Should we also bail on innerBlocks in this unusual case?
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.
No particular insight. IIRC it was copied over from Kevin's initial implementation.
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 bit of code had to change to accommodate client ID deduping of the whole tree, and I think this change makes sense in that context. I can also remove this small overhaul if you want to keep it the same, but it made more sense to me to dedupe across more than one level at a time.
SirLouen
left a comment
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.
@alecgeatches this patch is not applying for me. It's rebased to the lastest trunk?
I cannot find a cursorPosition in crdt-blocks-ts
https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/utils/crdt-blocks.ts
|
@SirLouen This PR is currently into our WordPress:wpvip/rtc-plugin branch, which we use to make changes upstream of actual Gutenberg You can see |
|
@alecgeatches is this an unofficial fork or something? Isn't planned to be merged into |
This branch leads our |
| const currentAttribute = | ||
| currentAttributes.get( attributeName ); | ||
| const isRichText = isRichTextAttribute( | ||
| block.name, | ||
| attributeName | ||
| ); |
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.
Could we conceivably have other attribute type transitions that we need to expect? Is there another more flexible approach that would allow us to add them in the future (and mark the location accordingly)?
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.
Addressed in cdfe518, let me know what you think! This adds isExpectedAttributeType() which is a less brittle way of doing the same check, and modifies our block attribute cache to store all expected block types, not just those for rich text.
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.
Actually, let me add one more change to do the same for the isRichText if statement.
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.
Added some more changes in b283ed9 to move type-specific logic into another function.
bacc977
into
WordPress:wpvip/rtc-plugin
…74304) * Add tests for mergeCrdtBlocks() edge cases * Fix potential bug if merged attribute has type change * Fix clientId deduping across block nesting levels * Move type checking in block attribute update to separate more-flexible function * Separate type-specific attribute logic into updateYBlockAttribute()
…sting (#74304) * Add tests for mergeCrdtBlocks() edge cases * Fix potential bug if merged attribute has type change * Fix clientId deduping across block nesting levels * Move type checking in block attribute update to separate more-flexible function * Separate type-specific attribute logic into updateYBlockAttribute()
…sting (#74304) * Add tests for mergeCrdtBlocks() edge cases * Fix potential bug if merged attribute has type change * Fix clientId deduping across block nesting levels * Move type checking in block attribute update to separate more-flexible function * Separate type-specific attribute logic into updateYBlockAttribute()
…sting (WordPress#74304) * Add tests for mergeCrdtBlocks() edge cases * Fix potential bug if merged attribute has type change * Fix clientId deduping across block nesting levels * Move type checking in block attribute update to separate more-flexible function * Separate type-specific attribute logic into updateYBlockAttribute()
…sting (#74304) * Add tests for mergeCrdtBlocks() edge cases * Fix potential bug if merged attribute has type change * Fix clientId deduping across block nesting levels * Move type checking in block attribute update to separate more-flexible function * Separate type-specific attribute logic into updateYBlockAttribute()
…sting (#74304) * Add tests for mergeCrdtBlocks() edge cases * Fix potential bug if merged attribute has type change * Fix clientId deduping across block nesting levels * Move type checking in block attribute update to separate more-flexible function * Separate type-specific attribute logic into updateYBlockAttribute()
As part of the work in
add/flattened-rtc-blocks, we're making significant changes to theY.Docblock data structure. This PR adds more complex and edge-case testing formergeCrdtBlocks(), and also fixes a couple of bugs found during the process.We will use these tests to reduce the risk that block data structure changes introduce merge errors.
Fixed bugs
The
handles block type changes from non-rich-text to rich-texttest was failing when an attribute transitioned from a raw string to rich-text (Y.Text) content when the string value was the same. The fix is theattributeHasTypeChangeboolean which ensure both the data type and content must be the same to skip a merge.Client Id deduplication was only running on a single level of blocks, which caused the
removes duplicate clientIds across different nesting levelstest. This meant a client ID could be freely duplicated as long as it was not in the same nesting level within the block structure. The code has been changed to:This required making an internal
mergeCrdtBlocksInternal()function that does the normal work of whatmergeCrdtBlocks()did previously, then run the top level logic after. This ensures the whole block structure of client IDs are deduplicated and avoids recursively making that check.Testing Instructions
To run the CRDT test suite: