Skip to content
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.

Sign up for GitHub

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

SelectControl: Infer value type from options #64069

Merged
merged 8 commits into from
Aug 6, 2024
Merged

SelectControl: Infer value type from options #64069

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ca2f43
SelectControl: Infer `value` type from `options`
mirka Jul 29, 2024
34ca3a0
Don't infer onChange
mirka Aug 1, 2024
ad20004
Add static type tests
mirka Aug 1, 2024
2ff5b37
Rename generic to `V` (for `value`)
mirka Aug 1, 2024
ee8e353
Merge branch 'trunk' into infer-select-control-types
mirka Aug 1, 2024
0770b14
Add changelog
mirka Aug 1, 2024
2f195c2
Simplify return type
mirka Aug 6, 2024
7488d9a
Merge branch 'trunk' into infer-select-control-types
mirka Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 20 additions & 15 deletions packages/components/src/date-time/time/index.tsx
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes we have to make in this file are what make me a bit hesitant about adding this value type inference. The stricter types do not break runtime code obviously, but we will be forcing some consumers to deal with this new strictness, which may seem like overkill depending on who you ask.

I think we can get away with it, because the strictness will only kick in if the options array is defined as immutable (either a literal array in the component call, or with an as const). For example, there is a potential bug in CustomGradientPicker but it isn't caught with the new strictness because GRADIENT_OPTIONS is mutable.

We don't have a ton of data points in the type checked realm of this repo, but it seems like TimePicker is the only "false positive" in the sense that it didn't surface a potential bug. (I just want to be sure that this type tightening is a net positive and not a nuisance 😅)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with what you're saying. It would likely be a good thing to document how to specify a loose type (by using SelectControl<string>()) so that users who don't want to tighten their other code (in this case, the format function) can still have the same level of leniency as they previously had, and then this change empowers users to use tighter types if they like.

Though, saying that, I'm not sure where this would be documented. In the SelectControl docs as a note perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to be sure that this type tightening is a net positive and not a nuisance 😅

This is definitely a big aspect to keep in mind. Overall, I think this change is a net positive — I hope that in the future we'll have a way to always apply the strictness, and not only when the options array is immutable.

We should probably write a dev note for this change, to help consumers of the package deal with this change correctly (especially the ones who need to support multiple versions of Wordpress / Gutenberg).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a Dev Note so we at least have something to link to if anybody asks. If we do get some questions, we might need to document it somewhere more permanent. But in general I don't think we should have "TypeScript documentation" aside from our actual type files.

Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,28 @@ export function TimePicker( {
);
}, [ currentTime ] );

const monthOptions = [
{ value: '01', label: __( 'January' ) },
{ value: '02', label: __( 'February' ) },
{ value: '03', label: __( 'March' ) },
{ value: '04', label: __( 'April' ) },
{ value: '05', label: __( 'May' ) },
{ value: '06', label: __( 'June' ) },
{ value: '07', label: __( 'July' ) },
{ value: '08', label: __( 'August' ) },
{ value: '09', label: __( 'September' ) },
{ value: '10', label: __( 'October' ) },
{ value: '11', label: __( 'November' ) },
{ value: '12', label: __( 'December' ) },
] as const;

const { day, month, year, minutes, hours } = useMemo(
() => ( {
day: format( date, 'dd' ),
month: format( date, 'MM' ),
month: format(
date,
'MM'
) as ( typeof monthOptions )[ number ][ 'value' ],
year: format( date, 'yyyy' ),
minutes: format( date, 'mm' ),
hours: format( date, 'HH' ),
Expand Down Expand Up @@ -146,20 +164,7 @@ export function TimePicker( {
__next40pxDefaultSize
__nextHasNoMarginBottom
value={ month }
options={ [
{ value: '01', label: __( 'January' ) },
{ value: '02', label: __( 'February' ) },
{ value: '03', label: __( 'March' ) },
{ value: '04', label: __( 'April' ) },
{ value: '05', label: __( 'May' ) },
{ value: '06', label: __( 'June' ) },
{ value: '07', label: __( 'July' ) },
{ value: '08', label: __( 'August' ) },
{ value: '09', label: __( 'September' ) },
{ value: '10', label: __( 'October' ) },
{ value: '11', label: __( 'November' ) },
{ value: '12', label: __( 'December' ) },
] }
options={ monthOptions }
onChange={ ( value ) => {
const newDate = setMonth( date, Number( value ) - 1 );
setDate( newDate );
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/query-controls/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ export function QueryControls( {
__next40pxDefaultSize={ __next40pxDefaultSize }
key="query-controls-order-select"
label={ __( 'Order by' ) }
value={ `${ orderBy }/${ order }` }
value={
orderBy === undefined || order === undefined
? undefined
: `${ orderBy }/${ order }`
}
Comment on lines +88 to +92
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an actual bug.

options={ [
{
label: __( 'Newest to oldest' ),
Expand Down
20 changes: 15 additions & 5 deletions packages/components/src/select-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ function useUniqueId( idProp?: string ) {
return idProp || id;
}

function UnforwardedSelectControl(
props: WordPressComponentProps< SelectControlProps, 'select', false >,
function UnforwardedSelectControl< T extends string >(
props: WordPressComponentProps< SelectControlProps< T >, 'select', false >,
ref: React.ForwardedRef< HTMLSelectElement >
) {
const {
Expand Down Expand Up @@ -66,12 +66,14 @@ function UnforwardedSelectControl(
const selectedOptions = Array.from( event.target.options ).filter(
( { selected } ) => selected
);
const newValues = selectedOptions.map( ( { value } ) => value );
const newValues = selectedOptions.map(
( { value } ) => value as T
);
props.onChange?.( newValues, { event } );
return;
}

props.onChange?.( event.target.value, { event } );
props.onChange?.( event.target.value as T, { event } );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These type assertions should be safe, as they will always match the options types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this isn't quite true because the select could be edited by someone using dev tools, however, I think this is fine because

  1. If you edit a website with dev tools, it's your fault you broke the code, not the code maintainer
  2. The likelihood of this happening is next to none

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be willing to use OptionType instead of T here? T doesn't really mean anything, and while it's a common practice in TypeScript, I see it as equivilent to naming a variable like const a = 'something'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed them to V (for value — it's not actually the option type), does that read a bit better? We don't have official naming conventions for this project yet, so eventually we may decide on a prefixed convention like TValue. But to write out a full name like Value, I think it's harder to signal that it's a generic. And ValueType is redundant because everything is a type 😅 We can revisit, but I hope V is a good compromise for now.

};

const classes = clsx( 'components-select-control', className );
Expand Down Expand Up @@ -164,6 +166,14 @@ function UnforwardedSelectControl(
* };
* ```
*/
export const SelectControl = forwardRef( UnforwardedSelectControl );
export const SelectControl = forwardRef( UnforwardedSelectControl ) as <
T extends string,
>(
props: WordPressComponentProps<
SelectControlProps< T >,
'select',
false
> & { ref?: React.Ref< HTMLSelectElement > }
) => ReturnType< typeof UnforwardedSelectControl >;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you were going for here, but I think I'd simplify the return type to just React.JSX.Element | null. That's the only thing that a component can ever return anyway, and it looks clearer to me this way. I don't really mind though, this is a nit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, thanks 🙏 2f195c2


export default SelectControl;
98 changes: 50 additions & 48 deletions packages/components/src/select-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import type { ChangeEvent, FocusEvent, ReactNode } from 'react';
import type { InputBaseProps } from '../input-control/types';
import type { BaseControlProps } from '../base-control/types';

type SelectControlBaseProps = Pick<
type SelectControlBaseProps< T extends string > = Pick<
InputBaseProps,
| '__next36pxDefaultSize'
| '__next40pxDefaultSize'
Expand All @@ -24,7 +24,7 @@ type SelectControlBaseProps = Pick<
Pick< BaseControlProps, 'help' | '__nextHasNoMarginBottom' > & {
onBlur?: ( event: FocusEvent< HTMLSelectElement > ) => void;
onFocus?: ( event: FocusEvent< HTMLSelectElement > ) => void;
options?: {
options?: readonly {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a readonly here so it allows immutable arrays. (Otherwise it would be a type error when passing an immutable array.)

/**
* The label to be shown to the user.
*/
Expand All @@ -33,7 +33,7 @@ type SelectControlBaseProps = Pick<
* The internal value used to choose the selected value.
* This is also the value passed to `onChange` when the option is selected.
*/
value: string;
value: T;
id?: string;
/**
* Whether or not the option should have the disabled attribute.
Expand Down Expand Up @@ -61,50 +61,52 @@ type SelectControlBaseProps = Pick<
variant?: 'default' | 'minimal';
};

export type SelectControlSingleSelectionProps = SelectControlBaseProps & {
/**
* If this property is added, multiple values can be selected. The `value` passed should be an array.
*
* In most cases, it is preferable to use the `FormTokenField` or `CheckboxControl` components instead.
*
* @default false
*/
multiple?: false;
value?: string;
/**
* A function that receives the value of the new option that is being selected as input.
*
* If `multiple` is `true`, the value received is an array of the selected value.
* Otherwise, the value received is a single value with the new selected value.
*/
onChange?: (
value: string,
extra?: { event?: ChangeEvent< HTMLSelectElement > }
) => void;
};
export type SelectControlSingleSelectionProps< T extends string = string > =
SelectControlBaseProps< T > & {
/**
* If this property is added, multiple values can be selected. The `value` passed should be an array.
*
* In most cases, it is preferable to use the `FormTokenField` or `CheckboxControl` components instead.
*
* @default false
*/
multiple?: false;
value?: NoInfer< T >;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoInfer<> is a new utility type added in TS 5.4, which tells TS not to use this field to infer the generic type. We need this because we want it to infer from the values in the options array, not the value prop.

This kind of "don't infer" signaling used to be done with T & string, but does not work anymore in TS 5.4+.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be demonstrated when the NoInfer can be useful? A new test case that fails without it? I don't understand very well what it does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Without NoInfer Typescript will infer the generic type from the value prop as well as the options prop.

What this means is it won't detect that your value is not one of the provided options, and therefore won't provide an error for it.

I've created a simple TS Playground example to illustrate the difference for you. Hope that helps 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is nice 🙂 I didn't realize that passing something as value can expand the T type to also include the type of value. But we want the type to be checked against options instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see a clever usage of NoInfer 👍 I had read about it, but struggled to think of a good use case for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also found a good article about NoInfer: https://www.totaltypescript.com/noinfer

/**
* A function that receives the value of the new option that is being selected as input.
*
* If `multiple` is `true`, the value received is an array of the selected value.
* Otherwise, the value received is a single value with the new selected value.
*/
onChange?: (
value: T,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also use the NoInfer utility for the onChange's arguments, in case a consumer of the component explicitly types onChange?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one, I added the constraints (and type tests) for those too.

extra?: { event?: ChangeEvent< HTMLSelectElement > }
) => void;
};

export type SelectControlMultipleSelectionProps = SelectControlBaseProps & {
/**
* If this property is added, multiple values can be selected. The `value` passed should be an array.
*
* In most cases, it is preferable to use the `FormTokenField` or `CheckboxControl` components instead.
*
* @default false
*/
multiple: true;
value?: string[];
/**
* A function that receives the value of the new option that is being selected as input.
*
* If `multiple` is `true`, the value received is an array of the selected value.
* Otherwise, the value received is a single value with the new selected value.
*/
onChange?: (
value: string[],
extra?: { event?: ChangeEvent< HTMLSelectElement > }
) => void;
};
export type SelectControlMultipleSelectionProps< T extends string > =
SelectControlBaseProps< T > & {
/**
* If this property is added, multiple values can be selected. The `value` passed should be an array.
*
* In most cases, it is preferable to use the `FormTokenField` or `CheckboxControl` components instead.
*
* @default false
*/
multiple: true;
value?: NoInfer< T >[];
/**
* A function that receives the value of the new option that is being selected as input.
*
* If `multiple` is `true`, the value received is an array of the selected value.
* Otherwise, the value received is a single value with the new selected value.
*/
onChange?: (
value: T[],
extra?: { event?: ChangeEvent< HTMLSelectElement > }
) => void;
};

export type SelectControlProps =
| SelectControlSingleSelectionProps
| SelectControlMultipleSelectionProps;
export type SelectControlProps< T extends string = string > =
| SelectControlSingleSelectionProps< T >
| SelectControlMultipleSelectionProps< T >;
Loading