Block Bindings: rely on broader context instead of requiring direct block context #60826
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. |
|
Size Change: +92 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
This PR is interesting, should we be doing the same for the backend as well? Or maybe it's already the case? |
| @@ -12,6 +12,7 @@ import { store as editorStore } from '../store'; | |||
| export default { | |||
| name: 'core/post-meta', | |||
| label: _x( 'Post Meta', 'block bindings source' ), | |||
| usesContext: [ 'postId', 'postType' ], | |||
There was a problem hiding this comment.
Where do we add these today (to block registration). I was expecting some code to be removed in this PR no?
There was a problem hiding this comment.
We add the context to the block type registration server side, so it will have to be removed there. I'm not sure if it's needed there or also added right in time. My understanding is that it can be adjusted too to not rely on the block type, cc @SantosGuillamot #58554 (comment)
There was a problem hiding this comment.
Right now, we are updating the uses_context when a source is registered: link. We did it that way to ensure the context was available in both the server and the editor, even when the binding was added afterward.
However, with this new approach, the editor side is solved, so I'd like to revisit it because that might not be necessary anymore.
There was a problem hiding this comment.
We need to do the same for backend sources. Fetch the actual global variable of the context or something like that. then we'd be able to remove the hook. It's too bad that it's not a "named hook" so we can't really unhook it from Gutenberg and we'd need to make the change on Core.
There was a problem hiding this comment.
I've been taking a look at the server part, and I believe we could add the following logic to the bindings processing here.
if ( ! empty( $block_binding_source->uses_context ) ) {
foreach ( $block_binding_source->uses_context as $context_name ) {
if ( array_key_exists( $context_name, $this->available_context ) ) {
$this->context[ $context_name ] = $this->available_context[ $context_name ];
}
}
}If we do that, we could get rid of the old method that changes uses_context when the source is registered.
I've tested it, and it seems to work fine. And it only adds the context to the blocks with bindings.
Is that what you had in mind?
There was a problem hiding this comment.
We considered both approaches in the WordPress 6.5 release cycle. We ended up going with the approach that exposes context for every block that might need it to work with the potentially established connections with block binding sources. It was a simpler approach as we didn't have to apply any changes on the client. The downside is that there is often context exposed even when it's not needed.
As before, I think the revised approach is also viable and presents different challenges. The biggest unknown exists on the server because the whole context for the current block gets computed in the constructor of the WP_Block:
It happens inside render_block, which is during the rendering phase, so that could have a positive impact on the block registration and editor's bootstrap. On the other hand, it's going to get processed for every individual block that gets rendered on the front end. The key question is if we can have a good strategy for getting the list of registered block sources and their expected context names.
There was a problem hiding this comment.
I've created a pull request with what I had in mind to change in core: WordPress/wordpress-develop#6456
The idea is to extend the context during the bindings processing by accessing the available_context. This way, the code only runs when the block has bindings and gets the needed context. The downside would be that it doesn't run in the editor, but that part is handled by this PR in Gutenberg.
I've tested it, and it seems to work.
There was a problem hiding this comment.
This is interesting. I haven't seen this idea before:
// Add the necessary context defined by the source.
if ( ! empty( $block_binding_source->uses_context ) ) {
foreach ( $block_binding_source->uses_context as $context_name ) {
if ( array_key_exists( $context_name, $this->available_context ) ) {
$this->context[ $context_name ] = $this->available_context[ $context_name ];
}
}
}So that would only happen for existing block binding connections locally just before the moment the final values gets computer. That seems very performant.
There was a problem hiding this comment.
I tried to replicate what this PR does in the server and it's the closest thing I could think of. I also agree that it seems much more performant than the previous implementation.
There was a problem hiding this comment.
The more I think about it, the more I like it. Great teamwork with the proposed refactoring. It seems like it should be considered as progressive enhancement since the useContext is correctly set on block types on the server in WP 6.5, so any changes applied in the Gutenberg plugin won't change much which is great. In WP 6.6, when the block types aren't fed with the same useContext from the block binding sources all the time no matter if the context is consumed, then we will seamlessly move that handling to the WP_Block class. We might not even need to change anything in the back compat layer in the Gutenberg plugin.
| for ( const key of source.usesContext ) { | ||
| context[ key ] = blockContext[ key ]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Worth noting that we do the exact same for blocks basically:
gutenberg/packages/block-editor/src/components/block-edit/edit.js
Lines 56 to 64 in f5ee54f
SantosGuillamot
force-pushed
the
try/block-binding-remove-block-context
branch
from
0d34203 to
800c96b
Compare
There was a problem hiding this comment.
This PR looks good to me 🙂
As discussed here, it only handles the context in the editor. It should be complemented with this other pull request in core that does the same in the server. Until that PR gets merged, everything will keep working with the downside that we extend the context whenever a source is registered, making it accessible even when the block is not using bindings.
There was a problem hiding this comment.
Good job figuring out and convincing for an alternative approach regarding the handling for the additional block context coming as a requirement from Block Binding source. More on that in #60826 (comment).
One remaining open question - not a blocker for this PR - is how we share use_context to the client so we don't have to repeat the definition it when registering handlers on the client. Basically, I'm talking about the line that @youknowriad points in https://github.com/WordPress/gutenberg/pull/60826/files#r1569275101.
|
I've been looking at exposing the The idea is to expose it through the editor settings: link. And consume that in the client with const { __experimentalBlockBindings: serverBindingsConfig } =
select("core/editor").getEditorSettings();
const serverUsesContext = new Set(
serverBindingsConfig?.[source.name]?.usesContext
);
const editorUsesContext = new Set(source.usesContext);
const mergedUsesContext = Array.from(
serverUsesContext.union(editorUsesContext)
);
for (const key of mergedUsesContext) {
context[key] = blockContext[key];
}What I had in mind was to:
What do you think about it @ellatrix @gziolo ? If we consider it a proper solution I'm happy to push the commit to this PR or create a new one after merging this. |
That should work correctly when the source gets registered in the server and the client. What about the case when the source is only registered on the server? The API on the client is still private so we need to account for the fact that custom block bindings sources are registered only on the server. I think it's still a viable approach to read these editor settings, but we would have to cover it explicitly. The alternative, would be registering data exposed from the server as initial sources and then enhancing them with what gets registered on the client. That's quite similar to how we handle block types. |
Right now, as the editor APIs are private, and they can't use the I like the idea of registering data exposed from the server as initial sources and enhancing them in the client if they have been registered as well. Although I believe we could handle that in another PR. |
This PR alone is safe to land after rebasing it with the latest changes from |
|
Now that we are starting to work on 6.7 release, I wanted to revisit the proposal to improve how we handle the context in block bindings, to see if it can be addressed in this release cycle. Let's do a recap and discuss a potential path forward. I wasn't sure where to have this discussion, here or in the core PR. How it works right now in
|
In addition to contacting plugin authors as mentioned in this comment, could it be worth doing some tests with external sources to see if we glean any insight? |
Do you have any examples in mind? Theoretically, blocks shouldn't need the context until the binding to the specific source is created. Use cases where context is needed without the binding could be considered "unexpected". |
I’ve brainstormed a bit and nothing comes to mind that would be realistic or pressing. I guess we’d have to imagine that an author would define an external source then want to use say the postID or postType to perform some logic for all the blocks of a particular type in a post or template, but even in that scenario, it seems creating a custom block rather than relying on the core ones would make most sense, which would take that beyond the scope of block bindings (at least as they’re envisioned right now). |
|
I think we should close this PR and do this option:
|
|
I don't think this is the most important task on its own as the block context is correctly handled today. It's mostly a code quality change that can be done over time.
I agree with the reasoning. We could use that as an opportunity to expose all the registered sources on the server to the client. This way, we will be able to share the label and We have a similar use case for block types, where we bootstrap block types registered on the server before registering the rest of the settings on the client with: gutenberg/packages/blocks/src/store/private-actions.js Lines 8 to 22 in d05f98b The settings passed from the server always take precedence over the settings set on the client because the server is always considered a source of truth because WP filters allow modifying values. The remaining refactoring that changes how the context is exposed to the binding sources could get postponed to WP 6.8 or later. |
|
As suggested, I started a new pull request trying to handle this. It is built on top of #63470 (which is on top of #63117) which exposes the properties the source defines in the server and let us reuse the I'm reusing the code from this pull request, apart from other changes, so I'm happy to rebase this pull request and push whatever is necessary if you feel that'd be better. If not, I guess we could close this one and continue the work there. Whatever you prefer it's fine to me.
Regarding this, I still believe refactoring how the context is exposed is important. The way it works right now, it extends the context for all the supported blocks when the source is defined. This means that if a source is registered with Maybe it is not a big issue at this moment because we only support four core blocks and there aren't too many sources extending the context. But I can expect more sources once the editor APIs are open and the idea is to extend the list of blocks supported at some point. It could become a problem if this "bug" is not solved soon. Additionally, all the pieces are there and (apparently) working, so I'd like to land them for 6.7. |
|
The code in this pull request was included as part of Block Bindings: Improve how the context needed by sources is extended in the editor. Should we close this one? |
I think that sounds good. I'll go ahead and close this — please anyone feel free to reopen 🙏 |
What?
This PR removes the need for bound block to have the context that a binding needs. Instead we can look at the broader context. I've opted to require a binding source to declare the context it needs, just to avoid passing all context down to the callbacks (not absolutely required though). This is very similar to how a block needs to declare what context it needs.
Why?
Block bindings shouldn't have to affect the block registration, instead we can add the right context just in time.
I could see Bits work similarly: it can declare what context it needs without requiring the parent block to consume the context as well.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast