Block Bindings: Unify getValue/getValues and setValue/setValues APIs
#63185
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
[Type] New API
|
Size Change: +88 B (+0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
64 tasks
|
Flaky tests detected in 473b5aa. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9810788697
|
|
Can you confirm the server-side counterpart for completeness? I believe it is
It would be valuable to discuss all of these APIs together. I'm also thinking how much this should mirror the known patterns we have for updating and reading block attributes as props passed to React components and through the block editor store. |
There was a problem hiding this comment.
I think Option 1 is better. It's slightly more code to write, and will perhaps be harder to configure as a result, but conceptually it makes sense to me for the source to receive the binding data and perform the necessary logic.
Below is an initial thought. I'll keep looking at this PR as discussion evolves and it or the alternative PR becomes ready for review 😄
|
My opinion is that less functions, less maintenance, less learning is always better. A get/set values with a batch of just one item would do the same work than a get/set values for several ones. |
SantosGuillamot
force-pushed
the
try/block-bindings-unify-get-set-calls-apis
branch
2 times, most recently
from
5e35a55
to
020ef83
Compare
|
Let's go with Option 1: Have ONLY getValues and setValues. Based on the feedback, it seems the preferred option and I agree it makes more sense. We can always add individual After taking a look at how attributes are handled, there are two similar use cases:
It's true that those use cases are slightly different because the implementation is managed by Gutenberg, and in this case we will ask external developers to define With the implementation of this pull request we would have: getValues - LinkParams - Link
{
url: {
args: {
key: 'url_field'
}
},
title: {
args: {
key: 'text_field'
}
}
}Normally, we would have Return {
url: 'https://wp.org',
title: 'Custom field value',
}setValues - LinkParams - Link It receives the same params as {
url: {
args: {
key: 'url_field'
},
newValue: 'https://wp.org/2'
},
title: {
args: {
key: 'text_field'
},
newValue: 'update custom field value'
}
}Return
Regarding the server-side counterpart, you're right we only have I'd like to explore having a Apart from that, I plan to work on improving the code docs and the types to ensure it is clear how to use these functions before making them public. |
|
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. |
artemiomorales
approved these changes
Jul 17, 2024
|
We may need to take a look at how to make this API public without asking the developer to pass Good work @SantosGuillamot ! Feel free to merge when you feel is ok. |
SantosGuillamot
force-pushed
the
try/block-bindings-unify-get-set-calls-apis
branch
from
24c8e95
to
ce80330
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Initially discussed here.
I'm opening this pull request to discuss some of the editor APIs that should be exposed by block bindings and how to make them more coherent. Right now, we have:
getValue: To get the value of each connected attribute one by one.setValue: To update the value of each connected attribute one by one.setValues: To update the values of all the attributes connected to a specific source in batch.In the pull request to use block bindings in pattern overrides,
setValuesfunction was introduced because updating in batch was needed for this particular use case.This is a valid use case not only for pattern overrides, but also for other sources that might want to call an API only once to update all the attributes connected to it. And this not only happens for updating the attributes (
setValues), but to get the value as well (getValues). For example, in post meta we are usinggetValueandsetValue, which means we are calling the REST API for each attribute connected to post meta.I can think of two main options:
✅ (Selected option) Option 1: Have ONLY
getValuesandsetValues.Covered in this pull request.
Basically, this would require source developers to update the values in batch. They will receive an object with all of the block attributes that are bound to that specific source, and it's up to them to decide what to do.
The developer experience might be slightly more complicated, and maybe it is a bit harder conceptually.
You can see how each source would use the different possibilities in the alternative pull request:
getValuevsgetValues: link.setValuevssetValues: link.getValuevsgetValues: link.You can check the differences in pattern overrides and post meta functions.
🔴 (Discarded option) Option 2: Have
getValue,getValuesInBatch,setValue, andsetValuesInBatch.Covered in this other pull request
Another option would be to allow both options and let developers decide which one to use. This will give more flexibility, but at the same time, it implies having two functions for a really similar purpose, which could also be confusing. Additionally, it complicates the core logic, and it will probably require more maintenance.
I would love to know more opinions on which approach you would take or other alternatives I didn't consider.
Why?
As an effort to polish the existing block bindings APIs in order to make them public, I believe it makes sense to revisit the this part and make it coherent.
Testing Instructions
Automated tests should pass. Block bindings and pattern overrides should keep working the same way.