Skip to content

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

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

Merged
SantosGuillamot merged 15 commits into from
Jul 16, 2024
Merged

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

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d716b5f
Move bindings registration to editor provider
SantosGuillamot Jul 4, 2024
6f2ab12
Remove old bindings import
SantosGuillamot Jul 4, 2024
6cabef4
Remove sources from editor provider
SantosGuillamot Jul 4, 2024
4701034
Create registerBlockBindingsSource
SantosGuillamot Jul 4, 2024
db6f4d4
Export function as private
SantosGuillamot Jul 4, 2024
d776739
Create `registerCoreBlockBindingsSources` function
SantosGuillamot Jul 4, 2024
d6978d8
Register core sources in edit-post and edit-site
SantosGuillamot Jul 4, 2024
efc1450
Don't reuse existing source
SantosGuillamot Jul 4, 2024
2a314b1
Add comment explaining function
SantosGuillamot Jul 4, 2024
e8e248c
Change the object in registration
SantosGuillamot Jul 11, 2024
2c99727
Add a few checks more
SantosGuillamot Jul 12, 2024
715911d
Add more registration functions
SantosGuillamot Jul 12, 2024
2866f51
Add private action to remove binding source
SantosGuillamot Jul 12, 2024
422aef7
Add unit tests
SantosGuillamot Jul 12, 2024
c27eed8
Export all functions as private
SantosGuillamot Jul 12, 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
Prev Previous commit
Next Next commit
Don't reuse existing source
  • Loading branch information
@SantosGuillamot
SantosGuillamot committed Jul 4, 2024
commit efc1450a79df5b2bc2d87ba86299c4b4339f3090
12 changes: 3 additions & 9 deletions packages/blocks/src/api/registration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,25 +762,19 @@ export const unregisterBlockVariation = ( blockName, variationName ) => {
export const registerBlockBindingsSource = ( source ) => {
const {
name,
label: newLabel,
label,
getValue,
setValue,
setValues,
getPlaceholder,
canUserEditValue,
} = source;

// Sources could exist because they have been initialized in the server.
// Check if the source is already registered.
const existingSource = unlock(
select( blocksStore )
).getBlockBindingsSource( name );

// Override the label if provided.
const label = newLabel || existingSource?.label;

// If the getValue callback is already provided, it shouldn't be overriden.
// It can't rely on name or label because those could be initialized in the server.
if ( existingSource?.getValue ) {
if ( existingSource ) {
console.error(
Copy link
Contributor

@cbravobernal cbravobernal Jul 16, 2024

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

@SantosGuillamot SantosGuillamot Jul 16, 2024

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

@gziolo gziolo Jul 16, 2024

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

@SantosGuillamot SantosGuillamot Jul 16, 2024

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

@SantosGuillamot SantosGuillamot Jul 16, 2024

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

@cbravobernal cbravobernal Jul 16, 2024

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

@gziolo gziolo Jul 16, 2024

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

@SantosGuillamot SantosGuillamot Jul 16, 2024

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

@SantosGuillamot SantosGuillamot Jul 16, 2024

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.'
);
Expand Down