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: don't use hooks #60724

Merged
merged 3 commits into from
Apr 17, 2024
Merged

Block bindings: don't use hooks #60724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
989c234
Block bindings: don't use hooks
ellatrix Apr 13, 2024
293b6df
Polish
ellatrix Apr 17, 2024
06d7e49
Remove useSource references
ellatrix Apr 17, 2024
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
251 changes: 90 additions & 161 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
@@ -1,12 +1,11 @@
/**
* WordPress dependencies
*/
import { getBlockType, store as blocksStore } from '@wordpress/blocks';
import { store as blocksStore } from '@wordpress/blocks';
import { createHigherOrderComponent } from '@wordpress/compose';
import { useSelect } from '@wordpress/data';
import { useLayoutEffect, useCallback, useState } from '@wordpress/element';
import { useRegistry, useSelect } from '@wordpress/data';
import { useCallback } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import { RichTextData } from '@wordpress/rich-text';

/**
* Internal dependencies
Expand Down Expand Up @@ -56,181 +55,111 @@ export function canBindAttribute( blockName, attributeName ) {
);
}

/**
* This component is responsible for detecting and
* propagating data changes from the source to the block.
*
* @param {Object} props - The component props.
* @param {string} props.attrName - The attribute name.
* @param {Object} props.blockProps - The block props with bound attribute.
* @param {Object} props.source - Source handler.
* @param {Object} props.args - The arguments to pass to the source.
* @param {Function} props.onPropValueChange - The function to call when the attribute value changes.
* @return {null} Data-handling component. Render nothing.
*/
const BindingConnector = ( {
args,
attrName,
blockProps,
source,
onPropValueChange,
} ) => {
const { placeholder, value: propValue } = source.useSource(
blockProps,
args
);

const { name: blockName } = blockProps;
const attrValue = blockProps.attributes[ attrName ];

const updateBoundAttibute = useCallback(
( newAttrValue, prevAttrValue ) => {
/*
* If the attribute is a RichTextData instance,
* (core/paragraph, core/heading, core/button, etc.)
* compare its HTML representation with the new value.
*
* To do: it looks like a workaround.
* Consider improving the attribute and metadata fields types.
*/
if ( prevAttrValue instanceof RichTextData ) {
// Bail early if the Rich Text value is the same.
if ( prevAttrValue.toHTMLString() === newAttrValue ) {
return;
}

/*
* To preserve the value type,
* convert the new value to a RichTextData instance.
*/
newAttrValue = RichTextData.fromHTMLString( newAttrValue );
}

if ( prevAttrValue === newAttrValue ) {
export const withBlockBindingSupport = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
const registry = useRegistry();
const sources = useSelect( ( select ) =>
unlock( select( blocksStore ) ).getAllBlockBindingsSources()
);
const bindings = props.attributes.metadata?.bindings;
const { name, clientId, context } = props;
const boundAttributes = useSelect( () => {
if ( ! bindings ) {
return;
}

onPropValueChange( { [ attrName ]: newAttrValue } );
},
[ attrName, onPropValueChange ]
);

useLayoutEffect( () => {
if ( typeof propValue !== 'undefined' ) {
updateBoundAttibute( propValue, attrValue );
} else if ( placeholder ) {
/*
* Placeholder fallback.
* If the attribute is `src` or `href`,
* a placeholder can't be used because it is not a valid url.
* Adding this workaround until
* attributes and metadata fields types are improved and include `url`.
*/
const htmlAttribute =
getBlockType( blockName ).attributes[ attrName ].attribute;
const attributes = {};

for ( const [ attributeName, boundAttribute ] of Object.entries(
bindings
) ) {
const source = sources[ boundAttribute.source ];
if (
! source?.getValue ||
! canBindAttribute( name, attributeName )
) {
continue;
}

if ( htmlAttribute === 'src' || htmlAttribute === 'href' ) {
updateBoundAttibute( null );
return;
const args = {
registry,
context,
clientId,
attributeName,
args: boundAttribute.args,
};

attributes[ attributeName ] = source.getValue( args );

if ( attributes[ attributeName ] === undefined ) {
if ( attributeName === 'url' ) {
attributes[ attributeName ] = null;
} else {
attributes[ attributeName ] =
source.getPlaceholder?.( args );
}
}
}

updateBoundAttibute( placeholder );
}
}, [
updateBoundAttibute,
propValue,
attrValue,
placeholder,
blockName,
attrName,
] );

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

/**
* BlockBindingBridge acts like a component wrapper
* that connects the bound attributes of a block
* to the source handlers.
* For this, it creates a BindingConnector for each bound attribute.
*
* @param {Object} props - The component props.
* @param {Object} props.blockProps - The BlockEdit props object.
* @param {Object} props.bindings - The block bindings settings.
* @param {Function} props.onPropValueChange - The function to call when the attribute value changes.
* @return {null} Data-handling component. Render nothing.
*/
function BlockBindingBridge( { blockProps, bindings, onPropValueChange } ) {
const blockBindingsSources = unlock(
useSelect( blocksStore )
).getAllBlockBindingsSources();
const { setAttributes } = props;

return (
<>
{ Object.entries( bindings ).map(
( [ attrName, boundAttribute ] ) => {
// Bail early if the block doesn't have a valid source handler.
const source =
blockBindingsSources[ boundAttribute.source ];
if ( ! source?.useSource ) {
return null;
const _setAttributes = useCallback(
( nextAttributes ) => {
registry.batch( () => {
if ( ! bindings ) {
return setAttributes( nextAttributes );
}

return (
<BindingConnector
key={ attrName }
attrName={ attrName }
source={ source }
blockProps={ blockProps }
args={ boundAttribute.args }
onPropValueChange={ onPropValueChange }
/>
);
}
) }
</>
);
}

const withBlockBindingSupport = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
/*
* Collect and update the bound attributes
* in a separate state.
*/
const [ boundAttributes, setBoundAttributes ] = useState( {} );
const updateBoundAttributes = useCallback(
( newAttributes ) =>
setBoundAttributes( ( prev ) => ( {
...prev,
...newAttributes,
} ) ),
[]
);
const keptAttributes = { ...nextAttributes };

for ( const [
attributeName,
boundAttribute,
] of Object.entries( bindings ) ) {
const source = sources[ boundAttribute.source ];
if (
! source?.setValue ||
! canBindAttribute( name, attributeName )
) {
continue;
}

source.setValue( {
registry,
context,
clientId,
attributeName,
args: boundAttribute.args,
value: nextAttributes[ attributeName ],
} );
delete keptAttributes[ attributeName ];
}

/*
* Create binding object filtering
* only the attributes that can be bound.
*/
const bindings = Object.fromEntries(
Object.entries( props.attributes.metadata?.bindings || {} ).filter(
( [ attrName ] ) => canBindAttribute( props.name, attrName )
)
if ( Object.keys( keptAttributes ).length ) {
setAttributes( keptAttributes );
}
} );
},
[
registry,
bindings,
name,
clientId,
context,
setAttributes,
sources,
]
);

return (
<>
{ Object.keys( bindings ).length > 0 && (
<BlockBindingBridge
blockProps={ props }
bindings={ bindings }
onPropValueChange={ updateBoundAttributes }
/>
) }

<BlockEdit
{ ...props }
attributes={ { ...props.attributes, ...boundAttributes } }
setAttributes={ _setAttributes }
Copy link
Member

@kevin940726 kevin940726 May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if you have already been aware of this, but one reason we initially implemented Pattern Overrides differently is because patching setAttributes might not work in some edge cases.

There are different ways to update a block's attributes. Calling setAttributes from the <BlockEdit>'s props is just one of those ways. You can also call updateBlockAttributes directly from the block-editor store, updateBlock or replaceBlock to change the whole block, or even a replaceInnerBlocks and cloneBlock combo for whatever reasons.

We can make sure all core blocks favor setAttributes from the <BlockEdit>'s props whenever possible, but as we allow third-party blocks support, then it's probably going to be a problem.

A current example of this in the core block library would be the core/table-of-contents block. As you update the heading block elsewhere, then the ToC block will be updated using updateBlockAttributes.

The way we implemented in Pattern Overrides currently is the other way around. Instead of patching anything or "capturing" anything from the block updates, we "listen" to the changes that happened in the block and reflect the changes to the pattern instance (content attribute.) Everything that happens in the block is kept intact, so we don't have to patch anything. Hence, syncDerivedUpdates is used to "sync" the block's "updates" to the pattern instance. Actions wrapped in that action callback shouldn't create an undo level as it's merely syncing derived states between different blocks.

(Just for the info for #60721, c.c. @SantosGuillamot)

Copy link
Contributor

Choose a reason for hiding this comment

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

There are different ways to update a block's attributes. Calling setAttributes from the 's props is just one of those ways. You can also call updateBlockAttributes directly from the block-editor store, updateBlock or replaceBlock to change the whole block, or even a replaceInnerBlocks and cloneBlock combo for whatever reasons.

That's a good point but it's not an issue that is specific to pattern overrides. All block bindings have the same issue/impact here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm aware of it and this is actually something that not using hooks would allow us to fix. But I remember @SantosGuillamot saying it was not desirable to fix, I can't recall correctly. Please correct me if I'm wrong 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it is a good point and something we should aim to fix.

But I remember @SantosGuillamot saying it was not desirable to fix, I can't recall correctly.

Mmm, I don't remember exactly. Maybe we were discussing the possibility of not using the hook and moving it to the core directly, and I mentioned that it shouldn't be a blocker for WordPress 6.6. But I totally agree that should probably be the path to go.

/>
</>
);
Expand Down
4 changes: 3 additions & 1 deletion 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,7 +51,9 @@ export function registerBlockBindingsSource( source ) {
type: 'REGISTER_BLOCK_BINDINGS_SOURCE',
sourceName: source.name,
sourceLabel: source.label,
useSource: source.useSource,
getValue: source.getValue,
setValue: source.setValue,
getPlaceholder: source.getPlaceholder,
lockAttributesEditing: source.lockAttributesEditing,
};
}
4 changes: 3 additions & 1 deletion packages/blocks/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,9 @@ export function blockBindingsSources( state = {}, action ) {
...state,
[ action.sourceName ]: {
label: action.sourceLabel,
useSource: action.useSource,
getValue: action.getValue,
setValue: action.setValue,
getPlaceholder: action.getPlaceholder,
lockAttributesEditing: action.lockAttributesEditing ?? true,
},
};
Expand Down
38 changes: 11 additions & 27 deletions packages/editor/src/bindings/post-meta.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
/**
* WordPress dependencies
*/
import { useEntityProp } from '@wordpress/core-data';
import { useSelect } from '@wordpress/data';
import { store as coreDataStore } from '@wordpress/core-data';
import { _x } from '@wordpress/i18n';

/**
* Internal dependencies
*/
Expand All @@ -12,33 +12,17 @@ import { store as editorStore } from '../store';
export default {
name: 'core/post-meta',
label: _x( 'Post Meta', 'block bindings source' ),
useSource( props, sourceAttributes ) {
const { getCurrentPostType } = useSelect( editorStore );
const { context } = props;
const { key: metaKey } = sourceAttributes;
getPlaceholder( { args } ) {
return args.key;
},
getValue( { registry, context, args } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the backend, we have get_value_callback which receives "args" and the "block instance". while registry is fine and is a client only thing, I wonder if we should use the same argument between client and server.

const postType = context.postType
? context.postType
: getCurrentPostType();

const [ meta, setMeta ] = useEntityProp(
'postType',
context.postType,
'meta',
context.postId
);

if ( postType === 'wp_template' ) {
return { placeholder: metaKey };
}
const metaValue = meta[ metaKey ];
const updateMetaValue = ( newValue ) => {
setMeta( { ...meta, [ metaKey ]: newValue } );
};
: registry.select( editorStore ).getCurrentPostType();

return {
placeholder: metaKey,
value: metaValue,
updateValue: updateMetaValue,
};
return registry
.select( coreDataStore )
.getEditedEntityRecord( 'postType', postType, context.postId )
.meta?.[ args.key ];
},
};
Loading