Fix: InnerBlocks allowed blocks change with different sizes. #53943
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
jorgefilipecosta
added
[Type] Bug
jorgefilipecosta
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad,
ajitbohra and
ellatrix
as code owners
|
Size Change: +16 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4a41ed9eb7a6bc1ff2536083331a90eb07f400ff. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6076195296
|
youknowriad
reviewed
Aug 25, 2023
| const _allowedBlocks = useMemo( | ||
| () => allowedBlocks, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| allowedBlocks | ||
| [ allowedBlocks && allowedBlocks.length, ...allowedBlocks ] |
There was a problem hiding this comment.
I guess there's an issue here if allowedBlocks is undefined, the spreading breaks.
|
This will still generate a notice mentioned here #14515 (comment), no? |
|
If we want to avoid that warning and still not force users to "memoize" the array themselves, we could write a |
|
Maybe we should leave memoization to blocks/consumers? In most cases it’s easy as defining the value outside of the component and when blocks have reactive value they could handle this themselves. We can also show best practices in docs. |
I'd agree with that it was not a performance bottleneck in the editor. this particular memoization has potentially a bit impact on loading performance and would also impact typing a little bit. |
So we remove memoization? And we make sure none of our blocks creates a new reference every time? |
I'm saying the opposite personally. Write something that uses the "previous" value of allowedBlocks unless the new value is different using shallow comparison. |
jorgefilipecosta
force-pushed
the
fix/innerblocks-allowed-blocks-change-with-different-sizes
branch
from
57f56be
to
4a41ed9
Compare
youknowriad
reviewed
Sep 4, 2023
| // Store current value in ref if it is not shallow equal to the previous value. | ||
| useEffect( () => { | ||
| if( ! isShallowEqual( ref.current, value ) ) { | ||
| ref.current = value |
There was a problem hiding this comment.
The problem is that, this won't "re-render" the component, so the component is going to use the old value unless there's something else in the component that causes a "re-render".
Now, we could replace this with a state value instead of a ref value but it also has a small downside: it will render the component twice: once with the old value and once with the new value. I wonder if there's a way to avoid the two renders.
There was a problem hiding this comment.
Maybe we could use a state update during the render; that should avoid rendering twice. Cc @kevin940726
Ref: https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes
P.S. The isShallowEqual import is missing from the file.
Example, needs testing
function useShallowMemo( value ) {
const [ prevValue, setPrevValue ] = useState( value );
if ( ! isShallowEqual( prevValue, value ) ) {
setPrevValue( value );
}
return prevValue;
}There was a problem hiding this comment.
Thanks for sharing @Mamaduka just learned about this :)
jorgefilipecosta
force-pushed
the
fix/innerblocks-allowed-blocks-change-with-different-sizes
branch
3 times, most recently
from
bd1d3a8
to
ac098bf
Compare
|
I can reproduce the e2e test failure locally, manually testing the issue now. |
Mamaduka
reviewed
Sep 5, 2023
packages/e2e-tests/plugins/inner-blocks-allowed-blocks/index.js
Outdated
Show resolved
Hide resolved
Mamaduka
reviewed
Sep 5, 2023
jorgefilipecosta
force-pushed
the
fix/innerblocks-allowed-blocks-change-with-different-sizes
branch
from
d465cec
to
af99870
Compare
|
@jorgefilipecosta, it isn't clear if your recent |
jorgefilipecosta
force-pushed
the
fix/innerblocks-allowed-blocks-change-with-different-sizes
branch
from
af99870
to
d0d0b1b
Compare
Sorry I was not aware you submitted a fix, it was just a rebase to then work on the fix. I already re-added your fix, thank you a lot :) |
Mamaduka
approved these changes
Sep 5, 2023
jorgefilipecosta
deleted the
fix/innerblocks-allowed-blocks-change-with-different-sizes
branch
|
Thank you for the feedback, reviews, fixes, and all the help @Mamaduka and @youknowriad. |
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.
Fixes: #14515
The comparison we used for memorization did not take into account the arrays may change the size. This PR fixes the issue and updates the end-to-end test to include this case.
Testing
Verify the end-to-end tests pass.