Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Bindings: rely on broader context instead of requiring direct block context #60826

Closed
wants to merge 1 commit into from
Closed

Block Bindings: rely on broader context instead of requiring direct block context #60826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions packages/block-editor/src/hooks/use-bindings-attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@
import { store as blocksStore } from '@wordpress/blocks';
import { createHigherOrderComponent } from '@wordpress/compose';
import { useRegistry, useSelect } from '@wordpress/data';
import { useCallback } from '@wordpress/element';
import { useCallback, useContext } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import { unlock } from '../lock-unlock';
import BlockContext from '../components/block-context';

/** @typedef {import('@wordpress/compose').WPHigherOrderComponent} WPHigherOrderComponent */
/** @typedef {import('@wordpress/blocks').WPBlockSettings} WPBlockSettings */
Expand Down Expand Up @@ -58,11 +59,12 @@ export function canBindAttribute( blockName, attributeName ) {
export const withBlockBindingSupport = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
const registry = useRegistry();
const blockContext = useContext( BlockContext );
const sources = useSelect( ( select ) =>
unlock( select( blocksStore ) ).getAllBlockBindingsSources()
);
const bindings = props.attributes.metadata?.bindings;
const { name, clientId, context } = props;
const { name, clientId } = props;
const boundAttributes = useSelect( () => {
if ( ! bindings ) {
return;
Expand All @@ -81,6 +83,14 @@ export const withBlockBindingSupport = createHigherOrderComponent(
continue;
}

const context = {};

if ( source.usesContext?.length ) {
for ( const key of source.usesContext ) {
context[ key ] = blockContext[ key ];
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const context = useMemo( () => {
return blockType && blockType.usesContext
? Object.fromEntries(
Object.entries( blockContext ).filter( ( [ key ] ) =>
blockType.usesContext.includes( key )
)
)
: DEFAULT_BLOCK_CONTEXT;
}, [ blockType, blockContext ] );


const args = {
registry,
context,
Expand All @@ -102,7 +112,7 @@ export const withBlockBindingSupport = createHigherOrderComponent(
}

return attributes;
}, [ bindings, name, clientId, context, registry, sources ] );
}, [ bindings, name, clientId, registry, sources, blockContext ] );

const { setAttributes } = props;

Expand All @@ -127,6 +137,14 @@ export const withBlockBindingSupport = createHigherOrderComponent(
continue;
}

const context = {};

if ( source.usesContext?.length ) {
for ( const key of source.usesContext ) {
context[ key ] = blockContext[ key ];
}
}

source.setValue( {
registry,
context,
Expand All @@ -148,7 +166,7 @@ export const withBlockBindingSupport = createHigherOrderComponent(
bindings,
name,
clientId,
context,
blockContext,
setAttributes,
sources,
]
Expand Down
1 change: 1 addition & 0 deletions packages/blocks/src/store/private-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export function registerBlockBindingsSource( source ) {
type: 'REGISTER_BLOCK_BINDINGS_SOURCE',
sourceName: source.name,
sourceLabel: source.label,
usesContext: source.usesContext,
getValue: source.getValue,
setValue: source.setValue,
getPlaceholder: source.getPlaceholder,
Expand Down
1 change: 1 addition & 0 deletions packages/blocks/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ export function blockBindingsSources( state = {}, action ) {
...state,
[ action.sourceName ]: {
label: action.sourceLabel,
usesContext: action.usesContext,
getValue: action.getValue,
setValue: action.setValue,
getPlaceholder: action.getPlaceholder,
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/bindings/post-meta.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we add these today (to block registration). I was expecting some code to be removed in this PR no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

https://github.com/WordPress/wordpress-develop/blob/66b5d25be2b5e69833bacfe8f64de660ffc2bfa9/src/wp-includes/class-wp-block.php#L132-L153

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. I haven't seen this idea before:

https://github.com/WordPress/wordpress-develop/blob/13a63f2730e032967a13eb84081bfc9a9e45bb7a/src/wp-includes/class-wp-block.php#L268-L275

// 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

getPlaceholder( { args } ) {
return args.key;
},
Expand Down
Loading