-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Field API: move formatting logic to the field #73922
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.
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
Field API: move formatting logic to the field #73922
Conversation
|
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. |
|
Flaky tests detected in 42fb29c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20338230687
|
|
I've prepared #73924 (implements format for datetime) on top of this one. |
| const field = useMemo( () => { | ||
| const currentField = fields.find( ( f ) => f.id === filter.field ); | ||
| if ( currentField ) { | ||
| return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why we do this.. Aren't getValue and setValue already there in the normalized field? Even if they weren't, wouldn't we possibly override a fields getValue etc..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because the filters don't use an Item. We don't know what's the data shape at this point, we pass a new object with the value of the filter. That means we need to make sure getValue/setValue know how to work with this new object. Does this help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to put it, imagine the data shape was:
{
author: {
name: 'authorName'
}
}and the getValue implementation was:
getValue: ( { item } ) => item.author.name;
When we want to use the field to get the formatted value:
field.getValueFormatted( { [ field.id ]: filterInView.value }, field )
how do we know the shape of the data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not following myself either (the last part of your explanation is a bit unclear)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still don't get it, sorry. In your example how can we ensure the proper field value is displayed?
Also, even if it getValue is needed, is it the same for setValue? Why do we need that in filters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've discussed this on video with Nik. @youknowriad I'm going to share a better example here, but happy to hop on a quick call to clarify this.
This is the consumer code:
// Consumer code:
//
// - data
// - fields
// - etc.
type Item = {
author: {
publishedDate: string;
};
};
const data: Item = {
author: {
publishedDate: "2025/12/01",
},
};
const publishedDateField = {
id: "publishedDate",
getValue: (item: Item) => item.author.publishedDate,
getValueFormatted: (item: Item, field) => {
const value = field.getValue({ item });
const valueFormatted = /* do something to format value*/ value;
return valueFormatted;
},
};
const fields = [
publishedDateField,
// etc.
];At some point, the framework wants to work with it:
// Framework code: DataViews table, or DataForm layout, etc.
//
// It has access to data and fields, and retrieves the formatted value
// for a particular field.
const input = data;
const foundField = fields.find((f) => f.id === "publishedDate");
const value = foundField.getValueFormatted({ item: input });
// This works fine, and, value at this point is "December 1st, 2025".In the filters (also framework code), we also want the formatted value, but the input is not data, but some user selection. For example, it could be 2023/09/26:
// The same code as before won't work: item is a string, but it needs to be an object.
const input = "2023/09/26";
const foundField = fields.find((field) => field.id === "publishedDate");
const value = foundField.getValueFormatted({ item: input });
// This also doesn't work.
//
// We don't know how to build an Item object, so we just build one with the field id as props.
// The issue with this is that getValue expects the item to follow the Item type, but it doesn't.
//
// {
// publishedDate: "2023/09/26"
// }
const inputFromFilter = {
[foundField.id]: filterValue,
};
const value = foundField.getValueFormatted({ item: inputFromFilter });
// This works.
//
// We don't have a way to know how to build the Item object.
// So, instead, we override getValue to work with the object we know how to build:
const inputFromFilter = {
[foundField.id]: filterValue,
};
foundField.getValue = ({ item, field }) => item[field.id];
const value = foundField.getValueFormatted({ item: inputFromFilter });
// At this point, value is "September 26nd, 2023"Note this is the same we do for field.Edit to work as user filters (code, PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I see how that would work, but I can't help but think it could be a code smell. Maybe one of the interfaces or abstractions could be better and absorb this directly; in contrast, this current approach is — essentially, and correct me if I'm wrong — to mock items in a new arbitrary shape.
That said, these are just hollow suggestions from someone who barely touches these pieces. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't love it, but it makes sense and we shouldn't probably block this PR for this. We should consider though if we can improve it and the similar case for field.Edit.
Said that, setValue doesn't seem needed for this context though and we should remove it.
| import isValidRequiredForArray from './utils/is-valid-required-for-array'; | ||
| import isValidElements from './utils/is-valid-elements'; | ||
|
|
||
| function getValueFormatted< Item >( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename this to getFormattedValue? It feels more natural to me..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's better English, certainly :) I considered that name, but also thought this new method is extending getValue so I thought it'd be clearer that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinions, but still think getFormattedValue is better 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is exactly getValueFormatted, why do we need it? Why is it different than getValue? Is it the same thing as render ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getValuereturns the value from the underlying data (including hierarchical data):2025-01-01.renderdisplays a component with the given value, it can be anything (a badge, a button, etc.)getValueFormattedsits between the two and returns a value with the format applied:January 1st, 2025.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think rendering links, badges and things like that is most of the time something that should be lifted from the responsibility of the field into the framework (main field, badge fields...). I do realize that there are cases where they might still be necessary though but it would be good to reduce these as much as possible.
I think this is a ship that sailed. I suggested this at the beginning but didn't get support :)
I think getValueFormatted is equal render in 90% of the cases so creating a separate function is IMO confusing for users. I have a preference for a "prop" like render({config: {textOnly: true}}) (just like we have for media size for instance) as I don't expect this to be common.
Users won't have to provide anything in 90% of the cases, because the field type provides a default for them. When they do provide a render is because they actually want to display something that's not text. This means that 99% of the cases they write a custom render they have to handle two renders (one with and other without text).
With a dedicated formatter, they just provide their special render if they want to include links, etc. This approach has better API ergonomics, and it's free from consumer errors (e.g., consumers forgetting about the "textonly" render and the filters ending up with links).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very confusing to me that "render" is not used to render the fields wherever we want to render them, be it filters, data from, views. Anywhere. I think introducing an another rendering function is confusing and I think we should instead teach folks that "render" for a field is a generic thing to render the field. It's not about rendering the field in a specific context. It may have additional configurations like "mediaSize" or else, but there's only one render function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered alternatives to this approach, but none stands out as better or even okayish (all have strong cons, like render has).
The pipeline get => format => render is also very common in the industry, so I don't understand why we should not use this pattern:
- AG Grid: valueGetter, valueFormatter, cellRenderer.
- MUI: valueGetter, valueFormatter, renderCell.
These libraries address the same problem space as DataViews, they need to render components, render formatted strings, and access data from a source. The fact they use the same pattern proposed in this PR is a datapoint.
TanStack Table, which we used as inspiration at the beginning, uses accessor (getValue) and cell (render). It doesn't have the concept of a formatter. But note that they promote AG Grid as a better approach, which does include formatter. So I guess they learned something and find the formatter concept important now.
As of today, I don't see shippable alternatives to this. Happy to pair or explore any people suggest, and also hear what other people folks think of this conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I just don't understand what's preventing us from using "render" in the filters as well. For me "rendering" is for rendering everywhere. Otherwise it's not clear why in some places we use "render" to render a field, and in others we use "getFormattedValue" to render the field. Both of these are rendering the field, it becomes very confusing to reason about.
"cell rendering" is different, this is a specific function that is clear, if you want to render a field in a cell, use this function, otherwise use generic "render".
I don't really want to be a blocker to be honest. I do think this PR is better than trunk code wise... But in terms of API, I think it's confusing. Developers using fields to build custom layouts and things like that, need to know exactly how to "render" fields and not have two alternatives to pick from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get both points here but I think I'm leaning towards this approach. As @oandregal mentions:
This means that 99% of the cases they write a custom render they have to handle two renders (one with and other without text).
This means they would need to know about the textOnly config is etc.. In my mind the concept of formatted value is clearer, but maybe it's subjective.
For me "rendering" is for rendering everywhere.
As for @youknowriad's comment above, the framework should have some consistency on needed UI - filters in this case and not sure where else it could be used in future.. I don't see how such a UI (filters) could work by allowing arbitrary rendered elements/components.
What this discussion highlights for sure, is that we need to add documentation about it. We should also mention that if a field has a custom render and also formats, they should probably consider using the field.getValueFormatted in their render when displaying the field's value.
| | Required< FormatDate > | ||
| | Required< FormatInteger > | ||
| | Required< FormatNumber >; | ||
| getValueFormatted: ( value: any, field: NormalizedField< Item > ) => any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is normalized properly, is it? I'd expect in normalizeFields to also fallback to getValue (your get-value-formatted-default). That way we wouldn't need to pass that formatted default function to field types. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed this with Nik. His point was to make getValueFormatted optional in the field types (here, for example) and instead provide that default in the normalize function as a 2nd fallback (here).
What I mentioned was that all props for the field types are mandatory, so this follows what we do in every other property. If we were to change this, it'd be best to address it for all other properties. I actually prefer the types to be explicit so you don't have to look at multiple places to understand how they work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'd prefer if we could avoid some of the required default props in the field types, but we can consider it separately. I guess when implementing the registering field type API will be a good time to consider it.
096b967 to
c92a1a7
Compare
| if ( elements.length > 0 ) { | ||
| // When there are elements, we favor those | ||
| activeElements = elements.filter( ( element ) => { | ||
| if ( filter.singleSelection ) { | ||
| return element.value === filterInView?.value; | ||
| } | ||
| return filterInView?.value?.includes( element.value ); | ||
| } ); | ||
| } else if ( filterInView?.value !== undefined ) { | ||
| const field = fields.find( ( f ) => f.id === filter.field ); | ||
| let label = filterInView.value; | ||
| } else if ( Array.isArray( filterInView?.value ) ) { | ||
| // or, filterInView.value can also be array | ||
| // for the between operator, as in [ 1, 2 ] | ||
| const label = filterInView.value.map( ( v ) => { | ||
| const formattedValue = field?.getValueFormatted( { | ||
| item: { [ field.id ]: v }, | ||
| field, | ||
| } ); | ||
| return formattedValue || String( v ); | ||
| } ); | ||
|
|
||
| if ( field?.type === 'date' && typeof label === 'string' ) { | ||
| try { | ||
| const dateValue = parseDateTime( label ); | ||
| if ( dateValue !== null ) { | ||
| label = dateI18n( | ||
| ( field as NormalizedFieldDate< any > ).format.date, | ||
| getDate( label ) | ||
| ); | ||
| } | ||
| } catch ( e ) { | ||
| label = filterInView.value; | ||
| } | ||
| } else if ( field?.type === 'datetime' && typeof label === 'string' ) { | ||
| try { | ||
| const dateValue = parseDateTime( label ); | ||
| if ( dateValue !== null ) { | ||
| label = dateValue.toLocaleString(); | ||
| } | ||
| } catch ( e ) { | ||
| label = filterInView.value; | ||
| } | ||
| } else if ( field?.type === 'number' && typeof label === 'number' ) { | ||
| const numberField = field as NormalizedFieldNumber< any >; | ||
| label = formatNumber( label, numberField.format ); | ||
| } else if ( field?.type === 'integer' && typeof label === 'number' ) { | ||
| const integerField = field as NormalizedFieldInteger< any >; | ||
| label = formatInteger( label, integerField.format ); | ||
| } | ||
| activeElements = [ | ||
| { | ||
| value: filterInView.value, | ||
| // @ts-ignore | ||
| label, | ||
| }, | ||
| ]; | ||
| } else if ( typeof filterInView?.value === 'object' ) { | ||
| // or, it can also be object for the inThePast/over operators, | ||
| // as in { value: '1', units: 'days' } | ||
| activeElements = [ | ||
| { value: filterInView.value, label: filterInView.value }, | ||
| ]; | ||
| } else if ( filterInView?.value !== undefined ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of my next steps/follow-up is going to be refactor this logic: some things here need to be operator-aware and that may be moved to the operators itself. It's a bit involved and unrelated to field types, hence why I don't do it in this PR.
ntsekouras
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
c92a1a7 to
42fb29c
Compare
Part of #73548
What?
This PR refactors the DataViews filters and DataForm controls so that they are field type agnostic.
In the filters, move from this:
to (delegate the formatting to the field):
In the controls, move from:
to (any field whose format has
weekStartsOnwill be used, otherwise the default):Why?
To prepare for 3rd party type registration, see #73548
How?
getValueFormattedin the fields.getFormatin the field types in favor orformatso they are aligned with the fields.Testing Instructions
Visit the storybook and verify the field types still work as before.
Screen.Recording.2025-12-11.at.18.13.02.mov