-
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
Try to understand useBlockSync #57074
Conversation
Size Change: -25 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in c2831cd. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7212199478
|
// been reset we need to take the blocks again from the external | ||
// value property. | ||
pendingChanges.current.outgoing = []; | ||
setControlledBlocks(); |
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 I'm trying to understand if this change is fine or not. No tests are failing.
The areInnerBlocksControlled
state is only set by the setHasControlledInnerBlocks
action. And that action is only used twice in the code base, both in this file. It's only ever called once with false
, which is here. So we can simply move the code here instead of using useSelect
and a useEffect
?
I'm not sure how to test, and I'm not sure if there's e2e test missing 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.
Anyway, inner blocks are not controlled that often, so in terms of performance this is probably negligible. But still it's a bit simpler if possible.
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 function unsetControlledBlocks
is only called when the block list is unmounted. I know for sure that there are some block that can change from "uncontrolled" to "controlled" while being mounted. So I think this change is probably breaking something.
That said reading the code, I wonder if we could just do this instead const isControlled = ! clientId && value !== undefined;
to avoid the useSelect
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 forgot how to trigger it but I think the "navigation" block can move from uncontrolled to controlled while being mounted. I'm also certain that there is another use-case but can't find it right now.
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 subscription is not so important, it's only for controlled blocks anyway
What?
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast