Field API: make getValue and setValue declarative#74900
Conversation
oandregal
requested review from
ajitbohra,
fabiankaegy,
gigitux and
ntsekouras
as code owners
github-actions
Bot
added
[Package] Block library
oandregal
added
[Type] Enhancement
|
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. |
|
Size Change: +419 B (+0.01%) Total Size: 3.1 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0b5974b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/21352523807
|
| id: 'id', | ||
| url: 'url', | ||
| alt: 'alt', | ||
| backgroundType: 'mediaType', |
oandregal
force-pushed
the
update/make-get-set-value-declarative
branch
from
6de0e0e to
0b5974b
Compare
|
Can you expand a bit here. My first impression is that getValue and setValue are escape hatches already for advanced cases and the declarative alternative is to actually follow the shape of the data when defining the fields. For instance, why don't we have a "declarative" way to write "render" or "edit"... for me, this means we're creating a DSL and I don't think we should open the pandora box there. I guess I still don't understand why we don't want to accept these getValue, setValue usage for these legacy block attributes that should have been "type object" basically. |
|
Sure, I can expand. We already auto-generate many functions based on the field information:
{
id: 'field'
}
{
id: 'myField',
Edit: 'toggle' // change the default control
}{
id: 'myField',
Edit: { // configure the control (number of rows for the textarea)
control: 'textarea',
rows: 5
}
}This PR just adds a new option to auto-generate getValue/setValue functions. It enables users to do the same thing without writing functions.
My understanding is that we want to auto-generate the field info from the I already mentioned what I think about the object type: while useful, it doesn't absorb all use cases. Actually, none of the ones I've checked so far (BlockFields and WooSettings). Not all consumers can adapt its data source to conform to the Field API, and not every data source is neatly organized. Even if we were willing to update all block attributes to be neatly organized, that's a lot of work. This approach absorbs these use cases. It builds upon what we already have, and it doesn't introduce any new concept that would expand the API complexity (e.g., mapping props), and neither blurs the frontiers between APIs (e.g., group field type vs. DataForm API). It's just a more compact way to express the same. IMO, it's a great trade-off. |
|
With this PR and #74856 we'll be able to do #74409 (comment) (essentially absorb all non-declarative code from content-only blocks with the Field API), without introducing the group field type. |
Yes, but I don't think this should come at the expense of the Fields API, which I think these explorations are hurting a bit. They are making it more complex to deal with legacy baggage and I'd rather have migrations instead of that.
IMO, there's a big difference between this and the examples you share. The The getValue / setValue being "declarative" on the other hand is a DSL, we're inventing some conventions/config to tell DataViews how to extract and update a value from an object randomly. Right now, we support assigning random properties, and we support renaming properties (based on your proposal). What happens when someone later will say, now I want to call a function say "item.property = transform( newItem.updatedProperty ) |
|
My quick thoughts:
I need to make up my mind... :/ |
|
getValue and setValue do feel quite complicated to me. It was quite challenging to define it correctly for blocks. getValue is also always the opposite of setValue (at least for blocks, I'm interested to know if there's a situation where that isn't the case), so if there has to be a declarative way, I wonder if there's a way to make it easier. One observation is that gutenberg/packages/dataviews/src/dataform/stories/data-adapter.tsx Lines 91 to 99 in 8f69c4d The api that's used for mapping keys is I don't personally have strong feelings about where it's defined, if it's part of the field or form or something else. Also to mention that it might be possible to leave the mapping of attribute keys to later. That part could always remain a private API for blocks, while the other APIs that allow APIs to support fields are made public. Extenders can then try the APIs where it makes sense for them (e.g. new blocks that don't need any migration). It'd also be good to understand a timeline for doing that, perhaps it's good to focus on it as an early 7.1 feature. |
I'm trying to understand how this will improve the specific cases (image/audio blocks etc..) we want to update in this PR. The blocks updated still have non-serializable content with the Let's say we leave the current implementation in trunk with
I share Riad's concerns and wondering how hard it would be to just create the core blocks' deprecations and use the
If we went with this approach, how is different by having an new |
I don't think a deprecation would work, as the block would have the exact same markup as before, it's only the internal representation that changes. It might be possible to achieve through the My hunch is that it's not straightforward There are also some other concerns like bindings needing to be migrated, and I'm sure other concerns will arise too. I mentioned it before that it might be a good idea to try it in a PR for one or two blocks to get a feel for the complexity. (I don't have time to do it right now though) |
Why not? We have I'm not sure about the bindings TBH, but I agree some deprecation explorations would help here. |
They are seriously flawed APIs in my experience (see #63837 for an example of the problems they can introduce). If the plan is to use those functions I might choose to opt out of any involvement! |
|
What's the downside of just keeping some "specific" mapping code in our framework for these legacy blocks for now. We know it's contained hacky code until we learn more about how people are using this stuff. |
@ntsekouras Yes: we can't source the necessary configuration in block.json. This PR is just one of the steps, we'd still need #74942 to be able to remove the custom controls as well.
@youknowriad I feel where you're coming from, and I share the feeling against creating a DSL. Adding "future hypothetical changes" is not part of this PR, and I don't think it should ever be, so I'll leave that out of this conversation. Mapping is a real concern for the API: we need ways to map Edit controls to fields declaratively. Edit controls use their own data structure internally (e.g.,
@talldan getValue and setValue are the core mechanics of the API. The API doesn't work without them, and when they're not present they are auto-generated. I know you've added
@youknowriad The risk is that we create a new different API, like we had (args, map, etc.) instead of improving the official Field API. Mapping Edit and fields is a problem we'll also find in PHP-only blocks (and UI when we get to that). |
|
To sum up the stances I hear across all conversations we've had:
Keep your comments/ideas coming. I'm happy to answer them and investigate different paths. I'd also like to encourage and give others space to come up with their own proposals and implementations to address this problem. The timeline is quite tight for 7.0. |
I agree with this, current getValue/setValue solve this though. |
The Block Fields experiment (and code) is now completely decoupled from the Pattern Editing experiment (#74479), and I think it'd probably be best to explore only shipping Pattern Editing without Block Fields for 7.0. Block Fields could probably follow in 7.1 if enough progress is made. Just to say that I don't think it's something that needs lots of urgency. I think it'd be good to double check over everything to ensure APIs that should be private aren't shipping. That would include the PHP only blocks, which I haven't previously reviewed, but my guess is some parts are shipping as stable features currently. |
|
@oandregal do you have any thoughts about this part of my above comment?
I I think that's the important part that I'm trying to understand and if it's valid, we could still just use the existing |
|
@ntsekouras I responded to that here. Let me rephrase: for us to be able to source the existing field configuration in a JSON format (block.json, all of this), or in a PHP format (PHP-only blocks) we need a mechanism to map the data in use by the Edit control with the data from the fields (more context). Yes, getValue and setValue already serve to map Edit to the datasource. The problem is that they are not serializable. That's why the initial prototypes of the content-only blocks created their own APIs (mapping, args): they needed to solve that problem. So, for us to ship content-only, one part is removing the functions in getValue/setValue (what this PR is about). The other part is removing the edit controls, aka #74942 We need both PRs. Does this help? |
|
Does it make any difference, to all the parties involved and their respective opinions, if this property mapping idea is applied not to the getter and setter but as a property of the Edit config? {
id: 'myField',
Edit: {
control: 'MyControl',
args: { href: 'src', alt: 'description' },
},
}(With the understanding that this would make the field data more opaque to the system.) |
There is also a However, it's not currently used by blocks. |
@oandregal unfortunately not 😄 . Let me try to rephrase my question.
I get the above. My question is how does it help with the specific blocks like |
5 participants
Part of #73076
Related #73261, #74409
What?
Adds support for declarative property mapping syntax in the Field API's
getValueandsetValueoptions, allowing simple 1:1 property mappings to be expressed as objects instead of functions.Before:
After:
in addition to supporting functions, we also support objects
Why?
In
@wordpress/block-library, for the BlockFields/content-only experiment some blocks needed to definegetValue/setValuefunctions to make that work with the custom Edit components (media, link, rich-text). This is problematic because we want this info to be sourced fromblock.json, and so we need the field definition to be serializable (can't have JavaScript functions).By introducing a serializable version of
getValueandsetValueall blocks have removed the equivalent functions.How?
getValue/setValuetype definition take aRecord<string, string>, in addition to the existing function.normalizeFieldsto convert objects-basedgetValue/setValueto functions, via some new utilities:get-value-from-map.ts: converts agetValuemap to a function (syntax:{ outputKey: 'itemPath' })set-value-from-map.ts: converts asetValuemap to a function (syntax:{ itemKey: 'valueKey' })The implementation supports dot-path notation for nested properties (e.g., { name: 'user.profile.name' }).
Testing Instructions