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

Jump to bottom

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 all 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
27 changes: 15 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,17 @@ export function collections( state = {}, action ) {
}

export function blockBindingsSources( state = {}, action ) {
// Merge usesContext with existing values, potentially defined in the server registration.
const existingUsesContext = state[ action.name ]?.usesContext || [];
Copy link
Member

Choose a reason for hiding this comment

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

I had one realization afte this PR got merged. Currently, all reducers get executed whenever any action gets dispatched. In effect, the logic added on top of the function will get executed each time. I would recommend turning it into an util that gets called inside the switch in case it is really needed.

The following could get added as an util:

/**
 * Merges usesContext with existing values, potentially defined in the server registration.
 *
 * @param {string[]} existingUsesContext Existing `usesContext`.
 * @param {string[]} newUsesContext       Newly added `usesContext`.
 * @return {string[]|undefined} Merged `usesContext`.
 */
function getMergedUsesContext( existingUsesContext = [],  newUsesContext = [] ) {
	const mergedArrays = Array.from(
		new Set( existingUsesContext.concat( newUsesContext ) )
	);
	return mergedArrays.length > 0 ? mergedArrays : undefined;
}

That could be then used when updating reducers.

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 have created a follow-up pull request to discuss it and potentially fix it: link.

const newUsesContext = action.usesContext || [];
const mergedArrays = Array.from(
new Set( existingUsesContext.concat( newUsesContext ) )
);
const mergedUsesContext =
mergedArrays.length > 0 ? mergedArrays : 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 +399,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