Block Bindings: Improve the way block bindings sources are registered #63117
Conversation
SantosGuillamot
added
[Type] Code Quality
|
Size Change: -3.58 kB (-0.2%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
| @@ -223,6 +225,9 @@ export const ExperimentalEditorProvider = withRegistryProvider( | |||
| setRenderingMode, | |||
| } = unlock( useDispatch( editorStore ) ); | |||
| const { createWarningNotice } = useDispatch( noticesStore ); | |||
| const { registerBlockBindingsSource } = unlock( | |||
| useDispatch( blocksStore ) | |||
| ); | |||
There was a problem hiding this comment.
There's a subtlety here. The "blocks" store is a global store. The same store is used between all EditorProvider instances while the EditorProvider can also create multiple code/editor stores. (depending on a prop).
So the question to be asked here, what if two editor providers are loaded at the same time, do we want to call registerBlockBindingsSource twice. It's kind of the exact same reason why we don't register blocks here (in the EditorProvider) and instead we register them in edit-site/src/index and edit-post/src/index
So I'm wondering if we should just do like the block registration instead. All of these things (bindings, blocks, format library) are probably better registered in the same way.
There was a problem hiding this comment.
That would make sense. I'll try to move it to the edit-site and edit-post packages. Something I'm wondering is how external developers would hook into that once the APIs are public, but I can take a look at that as well.
Thanks a lot for the guidance!
There was a problem hiding this comment.
I've changed the approach trying to simulate something similar to how I understood block registration works. I've also updated the description of the pull request to reflect that. Basically, what I did:
Trying to follow a similar approach to how blocks are registered, I did:
- Create a new
registerBlockBindingsSourcefunction exported by@wordpress/blocks. There, I added some validation similar to the one we have in the server: link. - Create a new
registerCoreBlockBindingsSourcesfunction to register the current core sources, post meta and pattern overrides. This function uses the previous one and it is placed in@wordpress/editorbecause some sources, like post meta, might need to access the editor store. - Call the
registerCoreBlockBindingsSourcesfromedit-siteandedit-postas suggested.
The code should be simplified once the APIs are public and we should add more functions like unregisterBlockBindingsSource, but I believe that could be handled in another pull request.
Please, let me know if that's not what you had in mind.
SantosGuillamot
changed the title
|
This approach looks clean and makes sense to me — it feels good and is less cognitive overhead seeing |
|
I made a bunch of changes to make it more similar to how block types, variations, or collections are handled and I added a first set of unit tests similar to the ones we have in the server. As these APIs are still private, I'm marking this pull request as ready for review to start gathering more feedback. There are other pull requests that deal with the bindings editor APIs that may be affected by the outcome of this one. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| select( blocksStore ) | ||
| ).getBlockBindingsSource( name ); | ||
| if ( existingSource ) { | ||
| console.error( |
There was a problem hiding this comment.
Should this be translatable and dev only? Can be done in a follow-up.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can make that change 🙂
I just followed what is being done in the registerBlockType function: link.
There was a problem hiding this comment.
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
Warning
I'll create a follow-up pull request to properly discuss that.
There was a problem hiding this comment.
That's a good question. Interactivity API uses warnings, whether the interactivity worked or not.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Follow-up created here: #63610. Although I encountered some issues.
|
Merging this now. Happy to explore other paths if we receive feedback after merging. Good news is that the APIs are still private. |
| * | ||
| * @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. |
There was a problem hiding this comment.
Label should be optional as it can be provided from the server.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That works if you have a follow-up lined up.
There was a problem hiding this comment.
Yes, there is a follow-up already working. I just need to rebase and it should be ready.
| * getValue: () => 'Value to place in the block attribute', | ||
| * setValue: () => updateMyCustomValue(), | ||
| * setValues: () => updateMyCustomValuesInBatch(), |
There was a problem hiding this comment.
Have you decided on the final API? There was discussion about deciding about using with voices in favor of getValues and setValues.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think that's the challenge when documenting the current API that isn't considered a final one.
There was a problem hiding this comment.
I kept them open to leave time for feedback. I wanted to post a new comment with what I consider the conclusion.
There was a problem hiding this comment.
I have just posted this comment and marked it as ready for review.
| canUserEditValue: | ||
| action.canUserEditValue || ( () => false ), |
There was a problem hiding this comment.
What's the reasoning for providing a default handler only in this place? getValue, setValue, setValues ang getPlaceholder are all also optional so they will be undefined. Should the default handler for canUserEditValue be moved to the selector instead?
There was a problem hiding this comment.
For sources that aren't registered or don't provide that property, we want to lock the editing when a paragraph (or other block) is connected to it.
I didn't think about moving it to the selector, I can give that a try. I guess it would require changing both getAllBlockBindingsSources and getBlockBindingsSource: link.
There was a problem hiding this comment.
I created a follow-up pull request for this as well: link. I didn't move the logic to the selector, but I think I prefer that approach better. Let me know what you think 🙂
|
It's nice to see how the API is shaping up. There is still some follow-up work necessary, but for me, it's very close to what we can see in WordPress 6.7. Great progress. |
…WordPress#63117) * Move bindings registration to editor provider * Remove old bindings import * Remove sources from editor provider * Create registerBlockBindingsSource * Export function as private * Create `registerCoreBlockBindingsSources` function * Register core sources in edit-post and edit-site * Don't reuse existing source * Add comment explaining function * Change the object in registration * Add a few checks more * Add more registration functions * Add private action to remove binding source * Add unit tests * Export all functions as private Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: artemiomorales <artemiosans@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org>
What?
This pull request changes the way block bindings sources are registered to follow the same pattern as
blockType,blockVariation,blockCollection, etc, and much more similar to how it is handled in the server.Why?
The current way of registering them doesn't feel right. The registration currently lives outside of a hook or a component in the editor package: link. And it uses directly actions and selectors defined in the store.
How?
Trying to follow a similar approach to how blocks are registered, I did:
registerBlockBindingsSource,unregisterBlockBindingsSource,getBlockBindingsSource,getBlockBindingsSourcesfunctions similar to the existing ones for blockTypes exported by@wordpress/blocks. There, I added some validation similar to the one we have in the server: link.registerCoreBlockBindingsSourcesfunction to register the current core sources, post meta and pattern overrides. This function is similar toregisterCoreBlocks. It is placed in@wordpress/editorbecause some core sources, like post meta, might need to access the editor store.registerCoreBlockBindingsSourcesfromedit-siteandedit-postas suggested by Riad here.Follow-ups
Testing Instructions
Automated tests should pass.