useInnerBlockTemplateSync: cancel template sync on innerBlocks change or unmount #46307
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +25 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Dec 5, 2022
| // Only synchronize innerBlocks with template if innerBlocks are empty | ||
| // or a locking "all" or "contentOnly" exists directly on the block. | ||
| const currentInnerBlocks = getBlocks( clientId ); |
There was a problem hiding this comment.
I'm pretty sure there was a bug that forced me to use this selector here. In other words, something updated the inner blocks between the scheduling of the micro task and its execution. I can't recall exactly though and looking at the PR that introduced the microtask, it's not clear either.
There was a problem hiding this comment.
This approach also allows us to avoid reading currentInnerBlocks. Cancelling provides a guarantee that the innerBlocks from the closure are always valid.
It allows this only if the "effect cleanup" is triggered before the microtask. Is this a given?
There was a problem hiding this comment.
Is this a given?
Thanks for raising this. I investigated on a little toy project and the answer is that with a correct implementation of useSelect, yes, it should be a given. When the canonical store value changes, a store listener is called, useSelect calls forceRender() (which is an alias for setState), and that setState performs effect cleanup. If all of that happens synchronously within one event loop tick, then there is no room for another microtask to change the value.
Whether it happens synchronously depends on the exact setup of the environment. React 18 with legacy render (React 17) is sync, React 18 with createRoot is not. We'd have to use useSyncExternalStore in useSelect to make it sync again.
I'll try to rewrite the implementation to be more resilient and not require complex reasoning about React scheduling to prove that it's correct.
There was a problem hiding this comment.
I did some experiments also with useSyncExternalStore, use[Layout]Effect and queueMicrotask, and the conclusion is that innerBlocks and currentInnerBlocks can get out of sync quite easily. So I reverted the part of my patch where I removed currentInnerBlocks.
Now the patch just cancels the scheduled microtask every time the effect is cleaned up. Either on unmount, or when relevant data (inner blocks or template lock) change.
jsnajdr
force-pushed
the
update/inner-template-sync-cleanup
branch
from
b68c5c4 to
4e0f34c
Compare
youknowriad
approved these changes
Dec 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a little bug in
useInnerBlockTemplateSyncthat surfaces when running this React Native test:gutenberg/packages/block-library/src/buttons/test/edit.native.js
Lines 205 to 241 in c8516e6
The test creates a "Buttons" block and then it immediately and synchronously removes it. But meanwhile,
useInnerBlockTemplateSyncscheduled a microtask to populate the no-longer-existing block with the default template, i.e., one inner button. This microtask runs when the "Buttons" block no longer exists, dispatching thereplaceInnerBlocksaction on an invalidclientId. In the test, this triggers a "update not wrapped inact()" warning.I'm fixing this by cancelling the microtask when the effect dependencies change (mainly
innerBlocks) or when the component is unmounted. Only when theinnerBlocksvalue remains unchanged for at least one microtask tick, do we perform the sync.This approach also allows us to avoid reading
currentInnerBlocks. Cancelling provides a guarantee that theinnerBlocksfrom the closure are always valid.