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.

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

Extensibility: Make Block Bindings work with editor.BlockEdit hook (2nd try) #67523

Merged
merged 11 commits into from
Dec 11, 2024
229 changes: 218 additions & 11 deletions packages/block-editor/src/components/block-edit/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,27 @@ import clsx from 'clsx';
/**
* WordPress dependencies
*/
import { withFilters } from '@wordpress/components';
import {
getBlockDefaultClassName,
hasBlockSupport,
getBlockType,
hasBlockSupport,
store as blocksStore,
} from '@wordpress/blocks';
import { useContext, useMemo } from '@wordpress/element';
import { withFilters } from '@wordpress/components';
import { useRegistry, useSelect } from '@wordpress/data';
import { useCallback, useContext, useMemo } from '@wordpress/element';

/**
* Internal dependencies
*/
import BlockContext from '../block-context';
import isURLLike from '../link-control/is-url-like';
import {
canBindAttribute,
hasPatternOverridesDefaultBinding,
replacePatternOverridesDefaultBinding,
} from '../../utils/block-bindings';
import { unlock } from '../../lock-unlock';

/**
* Default value used for blocks which do not define their own context needs,
Expand Down Expand Up @@ -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;
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 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?

Copy link
Member Author

@gziolo gziolo Dec 11, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

https://github.com/WordPress/wordpress-develop/blob/32880ec4e910837cd267e1685733867b243e22fa/src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php#L503-L513

It was hardened in block.json schema:

"attributes": {
"description": "Attributes provide the structured data needs of a block. They can exist in different forms when they are serialized, but they are declared together under a common interface.\n\nSee the attributes documentation at https://developer.wordpress.org/block-editor/reference-guides/block-api/block-attributes/ for more details.",
"type": "object",
"patternProperties": {

However, when not provided on the server, that would still mean it's null.

On the client, it defaults to an empty object:

const blockType = {
name,
icon: BLOCK_ICON_DEFAULT,
keywords: [],
attributes: {},

However, I'm not entirely sure what happens if the server sets null when bootstraping the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@gziolo gziolo Dec 11, 2024

Choose a reason for hiding this comment

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

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:

https://github.com/WordPress/wordpress-develop/blob/32880ec4e910837cd267e1685733867b243e22fa/src/wp-includes/class-wp-block-type.php#L553-L563

const registry = useRegistry();
const blockType = getBlockType( name );
const blockContext = useContext( BlockContext );
const registeredSources = useSelect(
( select ) =>
unlock( select( blocksStore ) ).getAllBlockBindingsSources(),
[]
);
gziolo marked this conversation as resolved.
Show resolved Hide resolved

// Assign context values using the block type's declared context needs.
const context = useMemo( () => {
return blockType && blockType.usesContext
const { blockBindings, context, hasPatternOverrides } = useMemo( () => {
// Assign context values using the block type's declared context needs.
const computedContext = blockType?.usesContext
? Object.fromEntries(
Object.entries( blockContext ).filter( ( [ key ] ) =>
blockType.usesContext.includes( key )
)
)
: DEFAULT_BLOCK_CONTEXT;
}, [ blockType, blockContext ] );
// Add context requested by Block Bindings sources.
if ( attributes?.metadata?.bindings ) {
Object.values( attributes?.metadata?.bindings || {} ).forEach(
( binding ) => {
registeredSources[ binding?.source ]?.usesContext?.forEach(
( key ) => {
computedContext[ key ] = blockContext[ key ];
}
);
}
);
}
return {
blockBindings: replacePatternOverridesDefaultBinding(
name,
attributes?.metadata?.bindings
),
context: computedContext,
hasPatternOverrides: hasPatternOverridesDefaultBinding(
attributes?.metadata?.bindings
),
};
}, [
name,
blockType?.usesContext,
blockContext,
attributes?.metadata?.bindings,
registeredSources,
] );

const computedAttributes = useSelect(
( select ) => {
if ( ! blockBindings ) {
return attributes;
}

const attributesFromSources = {};
const blockBindingsBySource = new Map();

for ( const [ attributeName, binding ] of Object.entries(
blockBindings
) ) {
const { source: sourceName, args: sourceArgs } = binding;
const source = registeredSources[ sourceName ];
if ( ! source || ! canBindAttribute( name, attributeName ) ) {
continue;
}

blockBindingsBySource.set( source, {
...blockBindingsBySource.get( source ),
[ attributeName ]: {
args: sourceArgs,
},
} );
}

if ( blockBindingsBySource.size ) {
for ( const [ source, bindings ] of blockBindingsBySource ) {
// Get values in batch if the source supports it.
let values = {};
if ( ! source.getValues ) {
Object.keys( bindings ).forEach( ( attr ) => {
// Default to the the source label when `getValues` doesn't exist.
values[ attr ] = source.label;
} );
} else {
values = source.getValues( {
Copy link
Member Author

Choose a reason for hiding this comment

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

@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:

getValues( { select, context, bindings } ) {
const metaFields = getPostMetaFields( select, context );
const newValues = {};
for ( const [ attributeName, source ] of Object.entries( bindings ) ) {
// Use the value, the field label, or the field key.
const fieldKey = source.args.key;
const { value: fieldValue, label: fieldLabel } =
metaFields?.[ fieldKey ] || {};
newValues[ attributeName ] = fieldValue ?? fieldLabel ?? fieldKey;
}
return newValues;
},

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?

select,
context,
clientId,
bindings,
} );
}
for ( const [ attributeName, value ] of Object.entries(
values
) ) {
if (
attributeName === 'url' &&
( ! value || ! isURLLike( value ) )
) {
// Return null if value is not a valid URL.
attributesFromSources[ attributeName ] = null;
} else {
attributesFromSources[ attributeName ] = value;
}
}
}
}

return {
...attributes,
...attributesFromSources,
};
},
[
attributes,
blockBindings,
clientId,
context,
name,
registeredSources,
]
);

const setBoundAttributes = useCallback(
( nextAttributes ) => {
if ( ! blockBindings ) {
setAttributes( nextAttributes );
return;
}

registry.batch( () => {
const keptAttributes = { ...nextAttributes };
const blockBindingsBySource = new Map();

// Loop only over the updated attributes to avoid modifying the bound ones that haven't changed.
for ( const [ attributeName, newValue ] of Object.entries(
keptAttributes
) ) {
if (
! blockBindings[ attributeName ] ||
! canBindAttribute( name, attributeName )
) {
continue;
}

const binding = blockBindings[ attributeName ];
const source = registeredSources[ binding?.source ];
if ( ! source?.setValues ) {
continue;
}
blockBindingsBySource.set( source, {
...blockBindingsBySource.get( source ),
[ attributeName ]: {
args: binding.args,
newValue,
},
} );
delete keptAttributes[ attributeName ];
}

if ( blockBindingsBySource.size ) {
for ( const [
source,
bindings,
] of blockBindingsBySource ) {
source.setValues( {
select: registry.select,
dispatch: registry.dispatch,
context,
clientId,
bindings,
} );
}
}

const hasParentPattern = !! context[ 'pattern/overrides' ];

if (
// Don't update non-connected attributes if the block is using pattern overrides
// and the editing is happening while overriding the pattern (not editing the original).
! ( hasPatternOverrides && hasParentPattern ) &&
Object.keys( keptAttributes ).length
) {
// Don't update caption and href until they are supported.
if ( hasPatternOverrides ) {
delete keptAttributes.caption;
delete keptAttributes.href;
}
setAttributes( keptAttributes );
}
} );
},
[
blockBindings,
clientId,
context,
hasPatternOverrides,
setAttributes,
registeredSources,
name,
registry,
]
);

if ( ! blockType ) {
return null;
}

if ( blockType.apiVersion > 1 ) {
return <EditWithFilters { ...props } context={ context } />;
return (
<EditWithFilters
{ ...props }
attributes={ computedAttributes }
context={ context }
setAttributes={ setBoundAttributes }
/>
);
}

// Generate a class name for the block's editable form.
Expand All @@ -77,15 +282,17 @@ const EditWithGeneratedProps = ( props ) => {
: null;
const className = clsx(
generatedClassName,
attributes.className,
attributes?.className,
props.className
);

return (
<EditWithFilters
{ ...props }
context={ context }
attributes={ computedAttributes }
className={ className }
context={ context }
setAttributes={ setBoundAttributes }
/>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { useBlockRefProvider } from './use-block-refs';
import { useIntersectionObserver } from './use-intersection-observer';
import { useScrollIntoView } from './use-scroll-into-view';
import { useFlashEditableBlocks } from '../../use-flash-editable-blocks';
import { canBindBlock } from '../../../hooks/use-bindings-attributes';
import { canBindBlock } from '../../../utils/block-bindings';
import { useFirefoxDraggableCompatibility } from './use-firefox-draggable-compatibility';

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import FormatEdit from './format-edit';
import { getAllowedFormats } from './utils';
import { Content, valueToHTMLString } from './content';
import { withDeprecations } from './with-deprecations';
import { canBindBlock } from '../../hooks/use-bindings-attributes';
import { canBindBlock } from '../../utils/block-bindings';
import BlockContext from '../block-context';

export const keyboardShortcutContext = createContext();
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/hooks/block-bindings.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ import { useViewportMatch } from '@wordpress/compose';
import {
canBindAttribute,
getBindableAttributes,
} from '../hooks/use-bindings-attributes';
useBlockBindingsUtils,
} from '../utils/block-bindings';
import { unlock } from '../lock-unlock';
import InspectorControls from '../components/inspector-controls';
import BlockContext from '../components/block-context';
import { useBlockEditContext } from '../components/block-edit';
import { useBlockBindingsUtils } from '../utils/block-bindings';
import { store as blockEditorStore } from '../store';

const { Menu } = unlock( componentsPrivateApis );
Expand Down
1 change: 0 additions & 1 deletion packages/block-editor/src/hooks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import './metadata';
import blockHooks from './block-hooks';
import blockBindingsPanel from './block-bindings';
import './block-renaming';
import './use-bindings-attributes';
import './grid-visualizer';

createBlockEditFilter(
Expand Down
Loading
Loading