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

Block Bindings: Improve the way block bindings sources are registered #63117

Merged
merged 15 commits into from
Jul 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/blocks/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,10 @@ _Returns_

- `Array|string`: A list of blocks or a string, depending on `handlerMode`.

### privateApis

Undocumented declaration.

### rawHandler

Converts an HTML string to known blocks.
Expand Down
19 changes: 19 additions & 0 deletions packages/blocks/src/api/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
/**
* Internal dependencies
*/
import { lock } from '../lock-unlock';
import {
registerBlockBindingsSource,
unregisterBlockBindingsSource,
getBlockBindingsSource,
getBlockBindingsSources,
} from './registration';

// The blocktype is the most important concept within the block API. It defines
// all aspects of the block configuration and its interfaces, including `edit`
// and `save`. The transforms specification allows converting one blocktype to
Expand Down Expand Up @@ -164,3 +175,11 @@ export {
__EXPERIMENTAL_ELEMENTS,
__EXPERIMENTAL_PATHS_WITH_OVERRIDE,
} from './constants';

export const privateApis = {};
lock( privateApis, {
registerBlockBindingsSource,
unregisterBlockBindingsSource,
getBlockBindingsSource,
getBlockBindingsSources,
} );
175 changes: 175 additions & 0 deletions packages/blocks/src/api/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -758,3 +758,178 @@ export const registerBlockVariation = ( blockName, variation ) => {
export const unregisterBlockVariation = ( blockName, variationName ) => {
dispatch( blocksStore ).removeBlockVariations( blockName, variationName );
};

/**
* Registers a new block bindings source with an object defining its
* behavior. Once registered, the source is available to be connected
* to the supported block attributes.
*
* @param {Object} source Properties of the source to be registered.
* @param {string} source.name The unique and machine-readable name.
* @param {string} source.label Human-readable label.
Copy link
Member

Choose a reason for hiding this comment

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

Label should be optional as it can be provided from the server.

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 made it optional in the pull request to bootstrap the source from the server: link. Until then, I think label should be required, right?

Copy link
Member

Choose a reason for hiding this comment

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

That works if you have a follow-up lined up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is a follow-up already working. I just need to rebase and it should be ready.

* @param {Function} [source.getValue] Function to get the value of the source.
* @param {Function} [source.setValue] Function to update the value of the source.
* @param {Function} [source.setValues] Function to update multiple values connected to the source.
* @param {Function} [source.getPlaceholder] Function to get the placeholder when the value is undefined.
* @param {Function} [source.canUserEditValue] Function to determine if the user can edit the value.
*
* @example
* ```js
* import { _x } from '@wordpress/i18n';
* import { registerBlockBindingsSource } from '@wordpress/blocks'
*
* registerBlockBindingsSource( {
* name: 'plugin/my-custom-source',
* label: _x( 'My Custom Source', 'block bindings source' ),
* getValue: () => 'Value to place in the block attribute',
* setValue: () => updateMyCustomValue(),
* setValues: () => updateMyCustomValuesInBatch(),
Comment on lines +784 to +786
Copy link
Member

Choose a reason for hiding this comment

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

Have you decided on the final API? There was discussion about deciding about using with voices in favor of getValues and setValues.

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 feel confident about using just getValues and setValues. I was just rebasing the pull request to adapt the registration there: link. My plan is to make the changes needed, leave a comment sharing my opinion, and open it for review.

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't very clear what happened to the open discussions that didn't reach any conclusions. When landing changes, it would be helpful for all observers to explain the action plan for next steps in case they would like to continue following the work.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's the challenge when documenting the current API that isn't considered a final one.

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 kept them open to leave time for feedback. I wanted to post a new comment with what I consider the conclusion.

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 just posted this comment and marked it as ready for review.

* getPlaceholder: () => 'Placeholder text when the value is undefined',
* canUserEditValue: () => true,
* } );
* ```
*/
export const registerBlockBindingsSource = ( source ) => {
const {
name,
label,
getValue,
setValue,
setValues,
getPlaceholder,
canUserEditValue,
} = source;

// Check if the source is already registered.
const existingSource = unlock(
select( blocksStore )
).getBlockBindingsSource( name );
if ( existingSource ) {
console.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be translatable and dev only? Can be done in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? 🤔 I checked the rest of the console.error and console.warn Gutenberg has and it seems they are never translated. Maybe it is a good discussion to have globally? By the way, what do you mean with dev only?

Copy link
Member

Choose a reason for hiding this comment

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

The most optimal would be:

import { warning } from `@wordpress/warning`; 
warning( 'This is the message...' );

It's a hint for a developer, but the code continues to work after making the best informed decision.

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 can make that change 🙂

I just followed what is being done in the registerBlockType function: link.

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 am not sure when we should throw a warning and when an error. In both cases, the editor won't break but the bindings won't work as expected.

Error
Screenshot 2024-07-16 at 11 59 55

Warning
Screenshot 2024-07-16 at 11 59 10

I'll create a follow-up pull request to properly discuss that.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good question. Interactivity API uses warnings, whether the interactivity worked or not.

Copy link
Member

Choose a reason for hiding this comment

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

It would be great to unify. @wordpress/warning was introduced not that long ago, so historically, people would use console.error or console.warn based on their personal preferences. I would simplify it and use warning from @wordpress/warning as it doesn't add too much value on production anyway. The other benefit is that @wordpress/warning uses hooks internally so 3rd party plugin can intercept these warning. If I recall correctly @johnbillion integrated that into Query Monitor plugin that has 200k active installs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I will create a pull request to change all the console.error and console.warn in the registration file to use @wordpress/warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up created here: #63610. Although I encountered some issues.

'Block bindings source "' + name + '" is already registered.'
);
return;
}

// Check the `name` property is correct.
if ( ! name ) {
console.error( 'Block bindings source must contain a name.' );
return;
}

if ( typeof name !== 'string' ) {
console.error( 'Block bindings source name must be a string.' );
return;
}

if ( /[A-Z]+/.test( name ) ) {
console.error(
'Block bindings source name must not contain uppercase characters.'
);
return;
}

if ( ! /^[a-z0-9/-]+$/.test( name ) ) {
console.error(
'Block bindings source name must contain only valid characters: lowercase characters, hyphens, or digits. Example: my-plugin/my-custom-source.'
);
return;
}

if ( ! /^[a-z0-9-]+\/[a-z0-9-]+$/.test( name ) ) {
console.error(
'Block bindings source name must contain a namespace and valid characters. Example: my-plugin/my-custom-source.'
);
return;
}

// Check the `label` property is correct.
if ( ! label ) {
console.error( 'Block bindings source must contain a label.' );
return;
}

if ( typeof label !== 'string' ) {
console.error( 'Block bindings source label must be a string.' );
return;
}

// Check the `getValue` property is correct.
if ( getValue && typeof getValue !== 'function' ) {
console.error( 'Block bindings source getValue must be a function.' );
return;
}

// Check the `setValue` property is correct.
if ( setValue && typeof setValue !== 'function' ) {
console.error( 'Block bindings source setValue must be a function.' );
return;
}

// Check the `setValues` property is correct.
if ( setValues && typeof setValues !== 'function' ) {
console.error( 'Block bindings source setValues must be a function.' );
return;
}

// Check the `getPlaceholder` property is correct.
if ( getPlaceholder && typeof getPlaceholder !== 'function' ) {
console.error(
'Block bindings source getPlaceholder must be a function.'
);
return;
}

// Check the `getPlaceholder` property is correct.
if ( canUserEditValue && typeof canUserEditValue !== 'function' ) {
console.error(
'Block bindings source canUserEditValue must be a function.'
);
return;
}

return unlock( dispatch( blocksStore ) ).addBlockBindingsSource( source );
};

/**
* Unregisters a block bindings source
*
* @param {string} name The name of the block bindings source to unregister.
*
* @example
* ```js
* import { unregisterBlockBindingsSource } from '@wordpress/blocks';
*
* unregisterBlockBindingsSource( 'plugin/my-custom-source' );
* ```
*/
export function unregisterBlockBindingsSource( name ) {
const oldSource = getBlockBindingsSource( name );
if ( ! oldSource ) {
console.error(
'Block bindings source "' + name + '" is not registered.'
);
return;
}
unlock( dispatch( blocksStore ) ).removeBlockBindingsSource( name );
}

/**
* Returns a registered block bindings source.
*
* @param {string} name Block bindings source name.
*
* @return {?Object} Block bindings source.
*/
export function getBlockBindingsSource( name ) {
return unlock( select( blocksStore ) ).getBlockBindingsSource( name );
}

/**
* Returns all registered block bindings sources.
*
* @return {Array} Block bindings sources.
*/
export function getBlockBindingsSources() {
return unlock( select( blocksStore ) ).getAllBlockBindingsSources();
}
Loading
Loading