Block Bindings: do not use useSource hook conditionally #59403
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,16 @@ | |
| import { getBlockType, store as blocksStore } from '@wordpress/blocks'; | ||
| import { createHigherOrderComponent } from '@wordpress/compose'; | ||
| import { useSelect } from '@wordpress/data'; | ||
| import { useEffect, useCallback, useState } from '@wordpress/element'; | ||
| import { addFilter } from '@wordpress/hooks'; | ||
| import { RichTextData } from '@wordpress/rich-text'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { store as blockEditorStore } from '../store'; | ||
| import { unlock } from '../lock-unlock'; | ||
| import { useBlockEditContext } from '../components/block-edit/context'; | ||
|
|
||
| /** @typedef {import('@wordpress/compose').WPHigherOrderComponent} WPHigherOrderComponent */ | ||
| /** @typedef {import('@wordpress/blocks').WPBlockSettings} WPBlockSettings */ | ||
|
|
@@ -22,87 +25,255 @@ import { unlock } from '../lock-unlock'; | |
| * @return {WPHigherOrderComponent} Higher-order component. | ||
| */ | ||
|
|
||
| const BLOCK_BINDINGS_ALLOWED_BLOCKS = { | ||
| 'core/paragraph': [ 'content' ], | ||
| 'core/heading': [ 'content' ], | ||
| 'core/image': [ 'url', 'title', 'alt' ], | ||
| 'core/button': [ 'url', 'text', 'linkTarget' ], | ||
| }; | ||
|
|
||
| /** | ||
| * Based on the given block name, | ||
| * check if it is possible to bind the block. | ||
| * | ||
| * @param {string} blockName - The block name. | ||
| * @return {boolean} Whether it is possible to bind the block to sources. | ||
| */ | ||
| export function canBindBlock( blockName ) { | ||
| return blockName in BLOCK_BINDINGS_ALLOWED_BLOCKS; | ||
| } | ||
|
|
||
| /** | ||
| * Based on the given block name and attribute name, | ||
| * check if it is possible to bind the block attribute. | ||
| * | ||
| * @param {string} blockName - The block name. | ||
| * @param {string} attributeName - The attribute name. | ||
| * @return {boolean} Whether it is possible to bind the block attribute. | ||
| */ | ||
| export function canBindAttribute( blockName, attributeName ) { | ||
| return ( | ||
| canBindBlock( blockName ) && | ||
| BLOCK_BINDINGS_ALLOWED_BLOCKS[ blockName ].includes( 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 {any} props.attrValue - The attribute value. | ||
| * @param {string} props.blockName - The block 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, | ||
| attrValue, | ||
| blockName, | ||
| blockProps, | ||
| source, | ||
| onPropValueChange, | ||
| } ) => { | ||
| const { placeholder, value: propValue } = source.useSource( | ||
| blockProps, | ||
| args | ||
| ); | ||
|
|
||
| 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 ) { | ||
| return; | ||
| } | ||
|
|
||
| onPropValueChange?.( { [ attrName ]: newAttrValue } ); | ||
retrofox marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, | ||
| [ attrName, onPropValueChange ] | ||
| ); | ||
|
|
||
| useEffect( () => { | ||
|
There was a problem hiding this comment. The fact that this uses an effect means that there's a small time frame where the value passed to the block is incorrect. I think ultimately that could be a problem. So in other words, if there's a way to achieve the current PR without effects (useMemo and useCallback are fine), that would be preferred. There was a problem hiding this comment. Makes sense, I've started to address this by removing the I'm seeing an infinite loop so likely I'm missing a check somewhere to prevent the state update in some circumstances. cc @SantosGuillamot as this is probably the same issue as what you've seen. There was a problem hiding this comment. calling updateBoundAttribute won't work either, we can't update local state in render functions, they are supposed to be immutable. I think the ideal would be to find a way to have this: const { attributes, setAttributes } = useBoundAttributes( originalAttributes, originalSetAttributes )
return <BlockEditor attributes={ attributes } setAttributes={ setAttributes } />There was a problem hiding this comment. I'm not sure I have a good solution about how to implement this, it might mean chaining the There was a problem hiding this comment.
Indeed, I believe the
Yes, not using There was a problem hiding this comment.
Let's explore enhancements for it separately, given that it would be great to fix the blinking in WP 6.5. At the same time, it doesn't seem as big issue, as the fact that There was a problem hiding this comment. It should be possible to avoid using There was a problem hiding this comment. I opened a draft PR following this approach in #59443 |
||
| 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; | ||
|
|
||
| if ( htmlAttribute === 'src' || htmlAttribute === 'href' ) { | ||
| updateBoundAttibute( null ); | ||
| return; | ||
| } | ||
|
|
||
| updateBoundAttibute( placeholder ); | ||
| } | ||
| }, [ | ||
| updateBoundAttibute, | ||
| propValue, | ||
| attrValue, | ||
| placeholder, | ||
| blockName, | ||
| attrName, | ||
| ] ); | ||
|
|
||
| return null; | ||
| }; | ||
|
|
||
| /** | ||
| * 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 {string} props.blockName - The block name. | ||
| * @param {Object} props.blockProps - The BlockEdit props object. | ||
| * @param {Object} props.bindings - The block bindings settings. | ||
| * @param {Object} props.attributes - The block attributes. | ||
| * @param {Function} props.onPropValueChange - The function to call when the attribute value changes. | ||
| * @return {null} Data-handling component. Render nothing. | ||
| */ | ||
| function BlockBindingBridge( { | ||
| blockName, | ||
| blockProps, | ||
| bindings, | ||
| attributes, | ||
| onPropValueChange, | ||
| } ) { | ||
| const blockBindingsSources = unlock( | ||
| useSelect( blocksStore ) | ||
| ).getAllBlockBindingsSources(); | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| return ( | ||
| <BindingConnector | ||
| key={ attrName } | ||
| blockName={ blockName } | ||
| attrName={ attrName } | ||
| attrValue={ attributes[ attrName ] } | ||
| source={ source } | ||
| blockProps={ blockProps } | ||
| args={ boundAttribute.args } | ||
| onPropValueChange={ onPropValueChange } | ||
| /> | ||
| ); | ||
| } | ||
| ) } | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| const withBlockBindingSupport = createHigherOrderComponent( | ||
| ( BlockEdit ) => ( props ) => { | ||
| const { clientId, name: blockName } = useBlockEditContext(); | ||
| const { getBlockAttributes } = useSelect( blockEditorStore ); | ||
|
|
||
| /* | ||
| * Collect and update the bound attributes | ||
| * in a separate state. | ||
| */ | ||
| const [ boundAttributes, setBoundAttributes ] = useState( {} ); | ||
| const updateBoundAttributes = useCallback( | ||
| ( newAttributes ) => | ||
| setBoundAttributes( ( prev ) => ( { | ||
| ...prev, | ||
| ...newAttributes, | ||
| } ) ), | ||
| [] | ||
| ); | ||
|
|
||
| /* | ||
| * Create binding object filtering | ||
| * only the attributes that can be bound. | ||
| */ | ||
| const attributes = getBlockAttributes( clientId ); | ||
| const bindings = Object.fromEntries( | ||
| Object.entries( attributes.metadata?.bindings || {} ).filter( | ||
| ( [ attrName ] ) => canBindAttribute( props.name, attrName ) | ||
| ) | ||
| ); | ||
|
|
||
| return ( | ||
| <> | ||
| { Object.keys( bindings ).length > 0 && ( | ||
| <BlockBindingBridge | ||
| blockProps={ props } | ||
| blockName={ blockName } | ||
| bindings={ bindings } | ||
| attributes={ attributes } | ||
| onPropValueChange={ updateBoundAttributes } | ||
| /> | ||
| ) } | ||
|
|
||
| <BlockEdit | ||
| { ...props } | ||
| attributes={ { ...attributes, ...boundAttributes } } | ||
| /> | ||
| </> | ||
| ); | ||
| }, | ||
| 'withBlockBindingSupport' | ||
| ); | ||
|
|
||
| /** | ||
| * Filters a registered block's settings to enhance a block's `edit` component | ||
| * to upgrade bound attributes. | ||
| * | ||
| * @param {WPBlockSettings} settings - Registered block settings. | ||
| * @param {string} name - Block name. | ||
| * @return {WPBlockSettings} Filtered block settings. | ||
| */ | ||
| function shimAttributeSource( settings, name ) { | ||
| if ( ! canBindBlock( name ) ) { | ||
| return settings; | ||
| } | ||
|
|
||
| return { | ||
| ...settings, | ||
| edit: withBlockBindingSupport( settings.edit ), | ||
| }; | ||
| } | ||
|
|
||
| addFilter( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to address the pattern override use case and not push ourselves into a corder, we should also make use of the
updateValuecallback.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, the PR handled the updateValue callback but then decided to address it in a follow-up. Do you think we should proceed to do it in the same PR? cc @SantosGuillamot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is not going to be a public API and
updateValueis not going to be used yet, I would include it once we work on the pull request to use this hook for pattern overrides.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly,
updateValueis part of the API in the original PR #58085 and all prototypes. Post Meta source is forced to be in readonly mode for the initial rollout in WP 6.5.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that
updateValueis even implemented for the Post Meta source. It isn't wired here as intended for now.