Block Editor: Add a new hook for getting a stable block context object #47028
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: +208 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1904d75. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3900672955
|
Mamaduka
force-pushed
the
try/use-block-context
branch
from
1b591cf to
2a4af31
Compare
Mamaduka
commented
Jan 12, 2023
|
|
||
| // Wrap context provider if (and only if) block has context to provide. | ||
| const blockType = getBlockType( block.name ); | ||
| if ( blockType && blockType.providesContext ) { |
There was a problem hiding this comment.
This original condition was always true, so there's no harm in wrapping the block in a context provider similar to the web version. See #46729.
Mamaduka
commented
Jan 12, 2023
packages/block-editor/src/components/inner-blocks/use-block-context.js
Outdated
Show resolved
Hide resolved
Mamaduka
added
[Type] Performance
jsnajdr
reviewed
Jan 12, 2023
| const context = getBlockContext( block.attributes, blockType ); | ||
|
|
||
| blockList = ( | ||
| <LayoutProvider value={ layout }> |
There was a problem hiding this comment.
Seems to me you fixed a bug here: previously, if the component didn't provide a context, but did provide a layout, the LayoutProvider wasn't rendered and the layout wasn't applied.
I see quite a few blocks, very common ones, that are like this, i.e., their block.json has supports.__experimentalLayout field, but doesn't have providesContext. Buttons, Column, Columns, Group, ...
There was a problem hiding this comment.
Sort of. This was only working because blockType && blockType.providesContext was always truthy.
There was a problem hiding this comment.
Oh yes, so it was only potential, not real bug 👍
packages/block-editor/src/components/inner-blocks/use-block-context.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inner-blocks/use-block-context.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inner-blocks/use-block-context.js
Outdated
Show resolved
Hide resolved
jsnajdr
approved these changes
Jan 12, 2023
tyxla
approved these changes
Jan 12, 2023
| return undefined; | ||
| } | ||
|
|
||
| if ( Object.keys( blockType.providesContext ).length === 0 ) { |
There was a problem hiding this comment.
Any chance provideContext will not exist, causing Object.keys( undefined ) to crash?
There was a problem hiding this comment.
When providesContext doesn't exist in block.json, it will get defaulted to an empty object here.
There's only one edge case: if I specify providesContext: null in block.json, it will not get defaulted and Object.keys will really crash.
There was a problem hiding this comment.
Maybe it's worth adding a ?? {} or an additional non-nullish check just in case.
There was a problem hiding this comment.
I realized there's the providesContext: null case only after your comment.
There was a problem hiding this comment.
That sounds like intentionally breaking a block. What if someone sets a string instead of an object?
I think this is something that block register should handle.
There was a problem hiding this comment.
String is fine, it's an array-like object that has keys. Even number will be accepted. Only null or undefined will crash.
There was a problem hiding this comment.
I think this is something that block register should handle.
I don't disagree, after all, we probably have many places where we expect a string value in block.json, and do unguarded .includes or .startsWith calls. A malicious null will crash them, too.
There was a problem hiding this comment.
Agreed. That's why I approved the PR in the first place and considered this a minor thing. Leaving it to your judgement @Mamaduka.
There was a problem hiding this comment.
Thanks again for the great feedback. I will merge this as it is. But happy to add extra checks in the future. Maybe as a part of block registration.
geriux
added
the
Mobile App - Automation
geriux
approved these changes
Jan 13, 2023
geriux
removed
the
Mobile App - Automation
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.
What?
See #46797
PR adds a new hook for getting a stable block context object.
Why?
Currently, every attribute update causes the BlockContextProvider value to change. Providers and consumers should only care when attributes provided as context are updated.
How?
PR replaces
getBlockContextwith the newuseBlockContexthook (props @jsnajdr #46797 (comment)) that relies onuseSelectinternal memoization to ensure context's stable reference.Testing Instructions
Testing this improvement requires profiling React renders. In my tests, I've used Gallery block with few images while typing in Gallery block's caption field. The caption isn't provided via context, so it shouldn't cause context updates.
Below is a screencast of before and after profiles.
Screencast
CleanShot.2023-01-12.at.13.22.55.mp4