-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Field API: simplify field normalization #73387
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
Conversation
| }, | ||
| Edit: 'array', // Use array control | ||
| render, | ||
| getFormat: (): NormalizedFormat => ( {} ), |
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 this be named normalizeField or something and just does any "type" specific normalization?
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 pushed a more ambitious refactor along the lines we talked in the past. The "field type definition" concept is gone from the codebase, and, instead, there're normalizeField functions that take a Field and returns a NormalizedField.
This will help other parts of the codebase (e.g., validation) to be absorbed in those functions. It's now much clearer that the "field type definitions" are just factories — which wasn't very obvious before.
7957068 to
6ffdf13
Compare
|
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. |
| @@ -0,0 +1,17 @@ | |||
| const getValueFromId = | |||
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.
Extracted from the removed normalizedFields function verbatim.
| @@ -0,0 +1,17 @@ | |||
| const setValueFromId = | |||
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.
Extracted from the removed normalizeFields function verbatim.
| */ | ||
| import type { Field, FilterByConfig, Operator } from '../../types'; | ||
|
|
||
| function getFilterBy< 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.
Extracted from the removed normalizeFields function almost verbatim. The only change is that instead of receiving a field type definition as the second parameter, it now receives the concrete things that it needs.
| * @param fields Fields config. | ||
| * @return Normalized fields config. | ||
| */ | ||
| export default function normalizeFields< 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.
The function still exists, but it was moved here and it just delegates to the normalized function for the type.
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's a little unexpected to have this function here in its own file, especially now that it's hollowed out, while the closely related functions normalizeField and getNormalizeFieldFunction are in the index.
| isPrimary?: boolean; | ||
| } | ||
|
|
||
| export interface NormalizedFilterByConfig { |
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 all removed as it's no longer necessary.
| | 'url' | ||
| | 'array'; | ||
|
|
||
| /** |
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 all removed as it's no longer necessary.
| const arrayFieldType: FieldTypeDefinition< any > = { | ||
| sort, | ||
| isValid: { | ||
| const defaultOperators: Operator[] = [ OPERATOR_IS_ANY, OPERATOR_IS_NONE ]; |
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.
The changes for all field types are about converting a (template) object into a function that returns the actual (normalized) object.
62c07ab to
5f4488d
Compare
5f4488d to
c336ab3
Compare
|
Flaky tests detected in c336ab3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19532526294
|
|
@ntsekouras @youknowriad this is working well on my tests, and it's an improvement over what we have. Is there anything you want me to look or should we go ahead and merge? |
It would be helpful to me to review first. Can you wait a bit? I'll do it today. |
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.
This tests well, thank you! I've left some questions to better understand the rationale and the API itself. I'd defer approving to @youknowriad though, as he's more familiar with these parts.
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.
Finally with your comment here, I got it and can approve confidently :)
mcsf
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.
I'm sorry I'm late to the party. This looks good overall. I think it's moving in the right direction both in terms of managing the codebase/responsiblities and polishing the type system for the benefit of API consumers. And I appreciate the emphasis on readability!
I bet we can keep doing more.
| if ( typeof field.filterBy === 'object' ) { | ||
| let operators = field.filterBy.operators; | ||
|
|
||
| // Assign default values if no operator was provided. | ||
| if ( ! operators || ! Array.isArray( operators ) ) { | ||
| operators = defaultOperators; | ||
| } | ||
|
|
||
| // Make sure only valid operators are included. | ||
| operators = operators.filter( ( operator ) => | ||
| validOperators.includes( operator ) | ||
| ); | ||
|
|
||
| // If no operators are left at this point, | ||
| // the filters should be disabled. | ||
| if ( operators.length === 0 ) { | ||
| return false; | ||
| } | ||
|
|
||
| return { | ||
| isPrimary: !! field.filterBy.isPrimary, | ||
| operators, | ||
| }; | ||
| } | ||
|
|
||
| if ( defaultOperators.length === 0 ) { | ||
| return false; | ||
| } | ||
|
|
||
| return { | ||
| isPrimary: false, | ||
| operators: defaultOperators, | ||
| }; |
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.
A few things seemed a bit strange in this function: filtering the default operators (unless we really want that extra safety net to prevent user error?); a seemingly superfluous isArray check (unless we believe consumers are not using TS?); the amount of branching and return statements; and the cumbersome requirement for consumers that they provide two identical arrays (defaultOperators, validOperators).
For the latter, maybe that explicitness is welcome, but otherwise I could be a nice touch to let defaultOperators default to validOperators when it is undefined.
For the rest, I found this refactor clearer, but this is probably very subjective:
| if ( typeof field.filterBy === 'object' ) { | |
| let operators = field.filterBy.operators; | |
| // Assign default values if no operator was provided. | |
| if ( ! operators || ! Array.isArray( operators ) ) { | |
| operators = defaultOperators; | |
| } | |
| // Make sure only valid operators are included. | |
| operators = operators.filter( ( operator ) => | |
| validOperators.includes( operator ) | |
| ); | |
| // If no operators are left at this point, | |
| // the filters should be disabled. | |
| if ( operators.length === 0 ) { | |
| return false; | |
| } | |
| return { | |
| isPrimary: !! field.filterBy.isPrimary, | |
| operators, | |
| }; | |
| } | |
| if ( defaultOperators.length === 0 ) { | |
| return false; | |
| } | |
| return { | |
| isPrimary: false, | |
| operators: defaultOperators, | |
| }; | |
| const operators = | |
| field.filterBy?.operators?.filter( ( operator ) => | |
| validOperators.includes( operator ) | |
| ) ?? defaultOperators; | |
| // If no operators are left at this point, the filters should be disabled. | |
| if ( ! operators.length ) { | |
| return false; | |
| } | |
| return { | |
| isPrimary: !! field.filterBy?.isPrimary, | |
| operators, | |
| }; |
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 addressed this at 47f458c
| export default function getNormalizeFieldFunction< Item >( | ||
| type?: FieldType | ||
| ): FieldTypeDefinition< Item > { | ||
| ): ( field: Field< Item > ) => NormalizedField< 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.
Nit: a switch statement seems perfect for the body of this 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.
Follow-up at d12c817
| * @param fields Fields config. | ||
| * @return Normalized fields config. | ||
| */ | ||
| export default function normalizeFields< 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.
It's a little unexpected to have this function here in its own file, especially now that it's hollowed out, while the closely related functions normalizeField and getNormalizeFieldFunction are in the index.
| enableGlobalSearch: field.enableGlobalSearch ?? false, | ||
| enableHiding: field.enableHiding ?? true, | ||
| readOnly: field.readOnly ?? false, | ||
| filterBy: getFilterBy( field, defaultOperators, validOperators ), |
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.
Does this mean that we're making new copies of defaultOperators, validOperators and filterBy every time we normalise a field? If so, is the cost of that relevat, or negligible?
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.
#73546 is a follow-up to this PR. It changes things a bit.
| format?: FormatDate; | ||
| }; | ||
|
|
||
| export type NormalizedFormat = Required< FormatDate > | {}; |
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.
Unused, no?
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.
Addressed this at 5384d94
| weekStartsOn: | ||
| field.format?.weekStartsOn !== undefined && | ||
| DAYS_OF_WEEK.includes( field.format?.weekStartsOn as DayString ) | ||
| ? field.format.weekStartsOn | ||
| : numberToWeekStartsOn( getSettings().l10n.startOfWeek ), | ||
| }; |
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.
My apologies for asking something off topic, but what's the story behind the introduction of weekStartsOn (string), given that startOfWeek (integer) precedes it?
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.
In the preceding PR, I thought it'd be more convenient for consumers to set it as sunday | monday | ... instead of 0 | 1 | .... I can see how both approaches have pros/cons. Do you think we should just surface WordPress mechanism?
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.
The names are definitely a lot more convenient and clear, it's just that there could be some confusion, because throughout the codebase the property weekStartsOn is sometimes a string and sometimes an integer (e.g. DateCalendar, DateRangeCalendar) or an enum (useLilius).
But maybe the confusion is worth it. And maybe the way forward is to equip WP date to understand both formats. 🤷
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 prepared #73538 to fix a bug and revert back to numbers. It's a neat idea, but I realized now it needs to happen across all components.
|
I'm late to the party but for me this feels like it goes in the wrong direction. It makes reasoning about fields harder and push us further away from allowing folks to register third party field types (like they can register third-party block types) |
|
@youknowriad 🤔 I see we have two alternatives for that:
registerCustomFieldType(
'CUSTOM_TYPE',
{
filter: ( field ) => { /* ... */ }
validate: ( field ) => { /* ... */ }
format: ( field ) => { /* ... */ }
}
);
registerCustomFieldType(
'CUSTOM_TYPE,
customTypeFunction,
);Can you elaborate on why you have a preference for the former? Perhaps you're thinking of server-side custom field type registration? Since we have functions in both approaches, that is going to introduce some limitations whatever approach we use. |
|
The former is consistent with the Block API and any registration API we've had so far. It's not really clear to me why we go in another direction, the "why" is very unclear to me. |
|
@youknowriad I've prepared #73546 so that the field types return objects, and some specific functions would take the field as well (for now, just format). I had a preference for the normalization function for the field type because it allows the type to modify any aspect of the field + ergonomics., but it is not a strong opinion. We can revisit this later, if necessary. I've also created #73548 to track what needs to happen for 3rd parties to be able to register field types. |
@mcsf I've addressed this 13d787e#diff-098346a2473f918df906636042419228b26a9c8985c3d7772359072851d00ce5 (not sure why I can't comment in your comment). |
Follow-up to #72999
What?
Simplifies field normalization by delegating it to the field types.
Why?
This keeps a cleaner separation of concerns:
normalizeFieldsis a function that calls the proper type, which has all the information to normalize the field.For example, before this change,
normalizeFieldsneeded to do specific things for thedatetype that has a specific format no other field has, see #72999 (comment)How?
normalizeField( field<Item> ): NormalizedField<Item>.Testing Instructions
npm run storybook:dev) and smoke test.npm run dev) and smoke test the Pages, Templates, and Patterns screens in the Site Editor.