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: Allow bindings bootstrap after registration #63797

Merged
merged 5 commits into from
Jul 25, 2024
Merged

Block Bindings: Allow bindings bootstrap after registration #63797

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
14617a1
Add unit test
SantosGuillamot Jul 24, 2024
bec42c4
Keep existing properties in bootstrap
SantosGuillamot Jul 24, 2024
f8e0e33
Update test description
SantosGuillamot Jul 24, 2024
d5d070d
Reuse mergedContext
SantosGuillamot Jul 24, 2024
f296f91
Improve `mergedUsesContext` definition
SantosGuillamot Jul 24, 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
37 changes: 37 additions & 0 deletions packages/blocks/src/api/test/registration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,43 @@ describe( 'blocks', () => {
'Block bindings source "core/test-source" is already registered.'
);
} );

it( 'should correctly merge properties when bootstrap happens after registration', () => {
// Register source in the client.
const clientOnlyProperties = {
getValues: () => 'values',
setValues: () => 'new values',
getPlaceholder: () => 'placeholder',
canUserEditValue: () => true,
};
registerBlockBindingsSource( {
name: 'core/custom-source',
label: 'Client Label',
usesContext: [ 'postId', 'postType' ],
...clientOnlyProperties,
} );

// Bootstrap source from the server.
unlock(
dispatch( blocksStore )
).addBootstrappedBlockBindingsSource( {
name: 'core/custom-source',
label: 'Server Label',
usesContext: [ 'postId', 'serverContext' ],
} );

// Check that the bootstrap values prevail and the client properties are still there.
expect( getBlockBindingsSource( 'core/custom-source' ) ).toEqual( {
// Should use the server label.
label: 'Server Label',
// Should merge usesContext from server and client.
usesContext: [ 'postId', 'postType', 'serverContext' ],
// Should keep client properties.
...clientOnlyProperties,
} );

unregisterBlockBindingsSource( 'core/custom-source' );
} );
} );

describe( 'unregisterBlockBindingsSource', () => {
Expand Down
29 changes: 17 additions & 12 deletions packages/blocks/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,19 +372,19 @@ export function collections( state = {}, action ) {
}

export function blockBindingsSources( state = {}, action ) {
// Merge usesContext with existing values, potentially defined in the server registration.
let mergedUsesContext = [
Copy link
Member

@gziolo gziolo Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was playing with a more condensed syntax and this is what I came up with:

Screenshot 2024-07-24 at 09 28 08

What do you think about this alternative? Does it read better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this approach. There are a couple of problems though:

  • The existing usesContext and the new usesContext can be undefined, so we can't use them directly.
  • I belive we want to return undefined when both of them are undefined.

I made this change in this commit. I created different variables just for the sake of readability.

Let me know what you think and if that's clearer. For me, it definitely reads better.

Copy link
Member

Choose a reason for hiding this comment

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

What’s the reason there should be an undefined value? Could it default to an empty array otherwise? Would it simplify handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong reason to have an undefined value. I was just expecting it to be undefined if it is not defined in the server or in the client. But maybe we can default to an empty array.

Would it simplify handling?

We could remove this last step where we are checking it the array is empty.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/WordPress/wordpress-develop/blob/ef71a9ed3917d11cf0daec94bc8f3201959af1b8/src/wp-includes/class-wp-block-bindings-source.php#L53

I see it defaults to null on the server so maybe keeping it as is would be more consistent. Both work fine as long as the handling on the client is adjusted accordingly. Thank you for providing additional context.

...( state[ action.name ]?.usesContext || [] ),
...( action.usesContext || [] ),
];
// Remove duplicates.
mergedUsesContext =
mergedUsesContext.length > 0
? [ ...new Set( mergedUsesContext ) ]
: undefined;

switch ( action.type ) {
case 'ADD_BLOCK_BINDINGS_SOURCE':
// Merge usesContext with existing values, potentially defined in the server registration.
let mergedUsesContext = [
...( state[ action.name ]?.usesContext || [] ),
...( action.usesContext || [] ),
];
// Remove duplicates.
mergedUsesContext =
mergedUsesContext.length > 0
? [ ...new Set( mergedUsesContext ) ]
: undefined;

return {
...state,
[ action.name ]: {
Expand All @@ -401,8 +401,13 @@ export function blockBindingsSources( state = {}, action ) {
return {
...state,
[ action.name ]: {
/*
* Keep the exisitng properties in case the source has been registered
* in the client before bootstrapping.
*/
...state[ action.name ],
label: action.label,
usesContext: action.usesContext,
usesContext: mergedUsesContext,
},
};
case 'REMOVE_BLOCK_BINDINGS_SOURCE':
Expand Down
Loading