useBlockSync: avoid replacing blocks twice on mount #60967
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. |
ellatrix
changed the title
ellatrix
force-pushed
the
try/use-block-sync-avoid-setting-twice-on-mount
branch
from
8450bbd to
dda3f2a
Compare
|
Size Change: +44 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
| 'test', // It should use the given client ID. | ||
| fakeBlocks // It should use the controlled blocks value. | ||
| ); | ||
| expect( replaceInnerBlocks ).toHaveBeenCalledTimes( 1 ); |
There was a problem hiding this comment.
This test in particular will fail in trunk. It will have called replaceInnerBlocks twice on mount.
ellatrix
requested review from
jsnajdr,
noisysocks,
tyxla and
youknowriad
and removed request for
youknowriad
| if ( ! isMounted.current ) { | ||
| isMounted.current = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Here I'd like to ask what the isControlled value actually means: what's the purpose of areInnerBlocksControlled and setHasControlledInnerBlocks? The useBlockSync logic is one of the most intimidating parts of the block-editor store, so let's use this opportunity to clarify things.
Some observations from what I see in the code:
- The only place where
setHasControlledInnerBlocksis ever called is this veryuseBlockSynchook. And the only place whereisControlledcan go fromtruetofalseis on unmount, whenunsetControlledBlocksis called. Therefore, why doesuseBlockSyncneed thisuseEffecthook to detect changes when it it fully in control over these changes? It could do thependingChanges = []andsetControlledBlocks()calls imperatively, inunsetControlledBlocks. areInnerBlocksControlledis called insideuseBlockSync, and then additionally ingetBlocks,__experimentalGetPatternTransformItems,__unstableHasActiveBlockOverlayActiveandNavigationMenuContent. These usages are incomprehensible to me.
There was a problem hiding this comment.
"isControlled" means a block container controls its "block list". It has a dedicated "state" for the block list (value and onChange being passed to the InnerBlocks component).
It happens for thing like "reusable blocks", "post content" block... where the block list is in a separate entity than the main "block list" (root block list).
1- if you call getBlocks() without argument, you're expected to have the block list excluding all controlled inner blocks (for instance the blocks within a reusable block shouldn't be there)
2- if you call getBlocks( rootClientId ) and rootClientId is a controlled container (like a wp/block instance), you expect to get the inner blocks of that particular block (the ones excluded from 1)
I hope this helps.
There was a problem hiding this comment.
Now that I think about it, we should probably add this as "documentation" somewhere.
There was a problem hiding this comment.
I had to read this three times to get a rough grasp of what was going on. I agree that some documentation with background to clarify why and in which cases this is necessary would be useful.
I wonder if there's a better way to express in code that the mounted check should apply only to controlled blocks.
There was a problem hiding this comment.
So, this is how I understand it all works, in my own words. Please confirm or correct:
When blocks control their children, all the blocks are still part of one big block-editor tree, is that right? Or are there separate block-editor stores, created by context providers and nested registries?
When ControlledInnerBlocks mounts, it will start syncing the clientId subtree it manages, according to the value, and onChange props. When it unmounts, it stops syncing.
The controlled, synced blocks are stored in the block-editor's state.tree with controlled|| prefixes. It's basically as if there was a controlled branch on that part of the tree, overriding the "original" part of the tree, which is still there in state.tree, but stored with unprefixed clientId keys.
The setHasControlledInnerBlocks action marks a part of tree as controlled. Most directly it affects getBlocks, telling it which "branch" it should look at when returning inner blocks of a particular clientId. Then there is a handful of other places that check if a given clientId subtree is controlled.
From all this, it seems to me that the useBlockSync hook shouldn't read the isControlled value at all. And shouldn't have an useEffect( () => { ... }, [ isControlled ] ). Because the hook knows the value, it knows that the given clientId is controlled for the entire lifetime of the hook. That's what the hook is for! It sets setHasControlledInnerBlocks to true on mount, then does the syncing, and then sets setHasControlledInnerBlocks back to false when it unmounts.
There was a problem hiding this comment.
I think the whole useBlockSync only applies to controlled blocks? We never use that hooks for non controlled blocks.
There was a problem hiding this comment.
I agree -- why then does the hook read isControlled and does a setControlledBlocks call when it goes from true to false? It should do that call in its cleanup/unmount handler instead. Because that's the only place where isControlled can ever change from true to false.
There was a problem hiding this comment.
From all this, it seems to me that the useBlockSync hook shouldn't read the isControlled value at all. And shouldn't have an useEffect( () => { ... }, [ isControlled ] ). Because the hook knows the value, it knows that the given clientId is controlled for the entire lifetime of the hook. That's what the hook is for! It sets setHasControlledInnerBlocks to true on mount, then does the syncing, and then sets setHasControlledInnerBlocks back to false when it unmounts.
Your own words are correct, but I'm not 100% sure about this part but it's not that important.
I think the whole useBlockSync only applies to controlled blocks? We never use that hooks for non controlled blocks.
It might not be the case anymore, it used to be called for all InnerBlocks component.
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
I think this PR could use some testing instructions. I'm not sure I fully follow all the scenarios it could affect and for someone unfamiliar, there's a big chance something could be missed.
|
@tyxla The e2e test should serve as enough testing, with the addition of the extra checks I added in the unit tests. Everything should just work as expected in the e2e tests & unit tests. |
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
The fix is not wrong and it fixes the issue. But after the discussion I'm fairly sure that instead of complicating the isControlled effect and making it smarter, we should be able to simply remove it 🙂
|
Ok, let's go ahead and prevent the duplication, I can try removing the effect and see what fails. |
What? Why?
Currently two effects are setting blocks on mount, we should return early in one of those effects. This avoids blocks being replaced by the same blocks, and things like
REMOVE_BLOCKS_AUGMENTED_WITH_CHILDRENbeing called when removing the previous blocks.How?
We don't run one of the effects on mount.
Testing Instructions
The e2e tests and unit tests should pass, which have more than enough coverage for controlled block syncing.
Testing Instructions for Keyboard
Screenshots or screencast