Block bindings: don't use hooks #60724
Conversation
ellatrix
added
[Type] Code Quality
|
Size Change: -71 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
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. |
|
So to summarize, you're proposing to replace Overall, these new APIs seem a bit more limiting than |
| getPlaceholder( { args } ) { | ||
| return args.key; | ||
| }, | ||
| getValue( { registry, context, args } ) { |
There was a problem hiding this comment.
In the backend, we have get_value_callback which receives "args" and the "block instance". while registry is fine and is a client only thing, I wonder if we should use the same argument between client and server.
|
(I was skeptical when I read the title of the PR but I'm less skeptical now :P) |
|
@youknowriad I think placeholder was needed when you're editing a template. You could either leave the attribute empty, but for things like paragraph content there's a desire to placehold. Maybe there's alternative solutions. Yes, it's more limiting by design. I think it's better to lock this API down more since we're in the early stages of block hooks, we can always consider opening it up more with hooks and this local state technique if it's really needed. |
Right now, that API provides the possibility of defining what to show as a placeholder in different scenarios. For example when we are in a template or when we are unable to retrieve the value (the field doesn't exist). In the case of custom fields source, we are returning the meta key, but other sources could do something different. Having said that, I'm not convinced about how we are handling placeholders right now. Not sure how they should work, but I belive it is not related to this particular PR. We should probably explore that API in a different PR/issue. Regarding the PR, overall, this approach feels much better to me. It solves most of the issues we have with the current implementation. We don't need to use Additionally, if I understood @ella correctly, with small changes to the code, it could allow us to access all the needed I've been testing the different use cases for custom fields and pattern overrides and everything seems to work fine, and the PR passes the tests. I guess it is a matter of deciding how much we want to limit external sources. I feel I don't have enough context at this moment to say if they will need to use hooks or not. Maybe we can merge this approach because the APIs are still private, add an experimental flag in Gutenberg to open them (not sure if that's possible), and ask people to test it and create their own sources to see how limiting it is. |
|
This approach looks good to me for the moment. As you said it's a private API. |
ellatrix
force-pushed
the
try/block-bindings-without-hooks
branch
from
6fa790c
to
293b6df
Compare
| <BlockEdit | ||
| { ...props } | ||
| attributes={ { ...props.attributes, ...boundAttributes } } | ||
| setAttributes={ _setAttributes } |
There was a problem hiding this comment.
Not sure if you have already been aware of this, but one reason we initially implemented Pattern Overrides differently is because patching setAttributes might not work in some edge cases.
There are different ways to update a block's attributes. Calling setAttributes from the <BlockEdit>'s props is just one of those ways. You can also call updateBlockAttributes directly from the block-editor store, updateBlock or replaceBlock to change the whole block, or even a replaceInnerBlocks and cloneBlock combo for whatever reasons.
We can make sure all core blocks favor setAttributes from the <BlockEdit>'s props whenever possible, but as we allow third-party blocks support, then it's probably going to be a problem.
A current example of this in the core block library would be the core/table-of-contents block. As you update the heading block elsewhere, then the ToC block will be updated using updateBlockAttributes.
The way we implemented in Pattern Overrides currently is the other way around. Instead of patching anything or "capturing" anything from the block updates, we "listen" to the changes that happened in the block and reflect the changes to the pattern instance (content attribute.) Everything that happens in the block is kept intact, so we don't have to patch anything. Hence, syncDerivedUpdates is used to "sync" the block's "updates" to the pattern instance. Actions wrapped in that action callback shouldn't create an undo level as it's merely syncing derived states between different blocks.
(Just for the info for #60721, c.c. @SantosGuillamot)
There was a problem hiding this comment.
There are different ways to update a block's attributes. Calling setAttributes from the 's props is just one of those ways. You can also call updateBlockAttributes directly from the block-editor store, updateBlock or replaceBlock to change the whole block, or even a replaceInnerBlocks and cloneBlock combo for whatever reasons.
That's a good point but it's not an issue that is specific to pattern overrides. All block bindings have the same issue/impact here.
There was a problem hiding this comment.
I'm aware of it and this is actually something that not using hooks would allow us to fix. But I remember @SantosGuillamot saying it was not desirable to fix, I can't recall correctly. Please correct me if I'm wrong 🙂
There was a problem hiding this comment.
I agree it is a good point and something we should aim to fix.
But I remember @SantosGuillamot saying it was not desirable to fix, I can't recall correctly.
Mmm, I don't remember exactly. Maybe we were discussing the possibility of not using the hook and moving it to the core directly, and I mentioned that it shouldn't be a blocker for WordPress 6.6. But I totally agree that should probably be the path to go.
What?
I don't see much reason for the binding source API to be a hook. We can add use a simpler get function that selects data from the store and a set function dispatch the action.
To see how setting the value works, see #60725 and #60721.
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast