Extensibility: Make Block Bindings work with editor.BlockEdit hook (2nd try)
#67523
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: -104 B (-0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
I see this warning: I think it is why the context was handled that way. |
The problem is that this hook observers the changes from: values = source.getValues( {
select,
context: contextFromSources,
clientId,
bindings,
} );We need to figure out how to listen to these changes differently to get rid of the warning. |
|
I've explored a similar solution at #67524. Context is computed in the |
|
Flaky tests detected in bdf4397. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12142449699
|
gziolo
commented
Dec 3, 2024
|
The performance metrics currently look as follows: 
@ellatrix, can you verify if everything is on track? I'm not entirely sure what the confidence level in these results is, as @SantosGuillamot and I observe a bit of randomness in both directions on every new commit. |
gziolo
added
[Feature] Block bindings
[Type] Bug
gziolo
commented
Dec 3, 2024
|
Another snapshot from the performance CI job: 
It's looking really good. |
|
I carried over most of the refactoring from #67370 for Block Bindings utils. |
gziolo
commented
Dec 3, 2024
gziolo
commented
Dec 3, 2024
gziolo
requested review from
ellatrix,
ntwb,
nerrad,
ajitbohra and
tellthemachines
as code owners
|
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. |
@SantosGuillamot, do you still see this warning? How can I test it? |
|
I think it was solved moving the context to the useMemo call. I was just testing it opening a page with a connected block and checking the console. |
|
Another performance snapshot from CI: 
|
gziolo
changed the title
withBlockBindingSupporteditor.BlockEdit hook
gziolo
changed the title
editor.BlockEdit hookeditor.BlockEdit hook (2nd try)
|
Maybe enhancements should happen at the source in @ellatrix has a better understanding of performance intricacies for |
ellatrix
reviewed
Dec 4, 2024
| attributes?.metadata?.bindings | ||
| ), | ||
| [ attributes?.metadata?.bindings, name ] | ||
| ); |
There was a problem hiding this comment.
Can't this be moved within useSelect?
There was a problem hiding this comment.
I was looking at it before. I need to check again.
There was a problem hiding this comment.
I think it's used also by useCallback, but it could be moved within both maybe.
There was a problem hiding this comment.
In f192c04, I merged it with another useMemo instead.
|
Yeah, at some point we should make built it in instead of adding it in a hook. But if this doesn't add select calls unless a block is bound, it seems fine. I looked at the changes and didn't see any obvious concerns. |
|
The performance runs on each run can be a bit off... If you have concerns, what I do is do a few runs and then average them. Or we'll see it in the graph for sure after merge. |
ellatrix
reviewed
Dec 4, 2024
ellatrix
reviewed
Dec 4, 2024
ellatrix
reviewed
Dec 4, 2024
| const blockType = getBlockType( name ); | ||
| const blockContext = useContext( BlockContext ); | ||
| const registeredSources = useSelect( ( select ) => |
There was a problem hiding this comment.
The only slightly concerning thing is the extra blocks store sub. But this store usually almost never changes, so it's not as bad as a blockEditor store sub. I don't think it will affect the results.
gziolo
force-pushed
the
update/refactor-with-block-binding-support
branch
from
ec2d136 to
f192c04
Compare
gziolo
commented
Dec 9, 2024
| values[ attr ] = source.label; | ||
| } ); | ||
| } else { | ||
| values = source.getValues( { |
There was a problem hiding this comment.
@Mamaduka, as I'm looking at:
I'm wondering what the pattern should be for observing store changes when you expect that for the same set of dependencies, there are expected different results. In this particular case, source.getValues might read the value for another store, code example:
gutenberg/packages/editor/src/bindings/post-meta.js
Lines 77 to 89 in 5e3d575
In effect, where coreDataStore updates for Post Meta source, then it should compute a different result. I now realize it's a limitation of the current implementation. How can we improve it?
|
If performance looks ok and it works, this is fine with me :) |
SantosGuillamot
approved these changes
Dec 11, 2024
There was a problem hiding this comment.
From my side, this looks good 🙂 Everything seems to keep working as expected and e2e tests pass. I've also compared the performance locally, and it seems it doesn't impact:
| This branch | Trunk |
|---|---|
 |
 |
I just left a small comment that I am not sure if it could cause issues.
Apart from that, I think it needs to be rebased after these changes to solve the CI problems.
| @@ -48,27 +57,223 @@ const Edit = ( props ) => { | |||
| const EditWithFilters = withFilters( 'editor.BlockEdit' )( Edit ); | |||
|
|
|||
| const EditWithGeneratedProps = ( props ) => { | |||
| const { attributes = {}, name } = props; | |||
| const { name, clientId, attributes, setAttributes } = props; | |||
There was a problem hiding this comment.
In the previous declaration, attributes had a default empty object. Could this cause any issues if it is undefined? Maybe some developers are relying on it being an object in BlockEdit?
There was a problem hiding this comment.
I don't think there was ever guaranteed that attributes is going to be an object due to historical reasons. computedAttributes gets passed down instead, which is always an object when there are no bindings. Otherwise, it will be the same value as defined by the block. I'm not entirely sure why it would get mapped to an empty object in this place, but if we want to keep that, then we should use a stable reference instead of overriding it with a new empty object instance on every re-render.
There was a problem hiding this comment.
On the server, attributes can be set to null:
https://github.com/WordPress/wordpress-develop/blob/32880ec4e910837cd267e1685733867b243e22fa/src/wp-includes/class-wp-block-type.php#L169-L175
The same applies to REST API response:
It was hardened in block.json schema:
gutenberg/schemas/json/block.json
Lines 107 to 110 in af34d1e
However, when not provided on the server, that would still mean it's null.
On the client, it defaults to an empty object:
gutenberg/packages/blocks/src/store/process-block-type.js
Lines 235 to 239 in af34d1e
However, I'm not entirely sure what happens if the server sets null when bootstraping the value.
There was a problem hiding this comment.
I think for the block type processing, attributes will never be null because it defines some attributes that are common to all blocks: metadata and lock: https://github.com/WordPress/wordpress-develop/blob/32880ec4e910837cd267e1685733867b243e22fa/src/wp-includes/class-wp-block-type.php#L282-L285.
If null is a valid value for the BlockEdit, I'm fine keeping it this way.
There was a problem hiding this comment.
Good point, it's probably impossible to see attributes set to null on the client based on the fact it always contains global attributes on the server:
yogeshbhutkar
pushed a commit
to yogeshbhutkar/gutenberg
that referenced
this pull request
Dec 18, 2024
…(2nd try) (WordPress#67523) * Block Bindings: Refactor `withBlockBindingSupport` * Fix issues reported by CI * Move bindings handling to EditWithGeneratedProps * Move sources context to `useMemo` * Reuse `getBlockBindingsSources` * Add e2e test * Add missing dependencies to block bindings JS script in e2e test * Refactor block bindings utils * Final touches on EditWithGeneratedProps * Improve block bindings utils * Address feedback from code review --------- Co-authored-by: Mario Santos <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: Mamaduka <[email protected]>
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?
Second attempt for #67370.
It wasn't possible to use
editor.BlockEditfilter to extend the block with additional controls and successfully usesetAttributesorattributesfor connected attribute using Block Bindings.Why?
The way all the logic for enhancing
attributesandsetAttributeswas connected too deep in the React components chain. It was below the point whereeditor.BlockEditallows overriding the Edit component. In effect the defaultattributesandsetAttributeswere passed to the component as if it wasn't connected with Block Bindings mechanism.How?
Refactored the code to move the logic higher in the hierarchy so all required child blocks receive access to enhanced
attributesandsetAttributesprops.Testing Instructions
I included a new e2e test that covers the extensibility point for
editor.BlockEdithooks that uses the enhanced props correctly by consuming them through input field injected into inspector controls:The simplest way to test it is using the Gutenberg Test Block Bindings plugin included with e2e tests:
text_field_value.Content.