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

Jump to bottom

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 6 commits
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

- `TimeInput`: Expose as subcomponent of `TimePicker` ([#63145](https://github.com/WordPress/gutenberg/pull/63145)).
- `RadioControl`: add support for option help text ([#63751](https://github.com/WordPress/gutenberg/pull/63751)).
- `SelectControl`: Infer `value` type from `options` ([#64069](https://github.com/WordPress/gutenberg/pull/64069)).

### Internal

Expand Down
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< V extends string >(
props: WordPressComponentProps< SelectControlProps< V >, '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 V
);
props.onChange?.( newValues, { event } );
return;
}

props.onChange?.( event.target.value, { event } );
props.onChange?.( event.target.value as V, { event } );
};

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 <
V extends string,
>(
props: WordPressComponentProps<
SelectControlProps< V >,
'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;
125 changes: 125 additions & 0 deletions packages/components/src/select-control/test/select-control.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,129 @@ describe( 'SelectControl', () => {
expect.anything()
);
} );

/* eslint-disable jest/expect-expect */
describe( 'static typing', () => {
describe( 'single', () => {
it( 'should infer the value type from available `options`, but not the `value` or `onChange` prop', () => {
const onChange: ( value: 'foo' | 'bar' ) => void = () => {};

<SelectControl
value="narrow"
options={ [
{
value: 'narrow',
label: 'Narrow',
},
{
value: 'value',
label: 'Value',
},
] }
// @ts-expect-error onChange type is not compatible with inferred value type
onChange={ onChange }
/>;

<SelectControl
// @ts-expect-error "string" is not "narrow" or "value"
value="string"
options={ [
{
value: 'narrow',
label: 'Narrow',
},
{
value: 'value',
label: 'Value',
},
] }
// @ts-expect-error "string" is not "narrow" or "value"
onChange={ ( value ) => value === 'string' }
/>;
} );

it( 'should accept an explicit type argument', () => {
<SelectControl< 'narrow' | 'value' >
// @ts-expect-error "string" is not "narrow" or "value"
value="string"
options={ [
{
value: 'narrow',
label: 'Narrow',
},
{
// @ts-expect-error "string" is not "narrow" or "value"
value: 'string',
label: 'String',
},
] }
/>;
} );
} );

describe( 'multiple', () => {
it( 'should infer the value type from available `options`, but not the `value` or `onChange` prop', () => {
const onChange: (
value: ( 'foo' | 'bar' )[]
) => void = () => {};

<SelectControl
multiple
value={ [ 'narrow' ] }
options={ [
{
value: 'narrow',
label: 'Narrow',
},
{
value: 'value',
label: 'Value',
},
] }
// @ts-expect-error onChange type is not compatible with inferred value type
onChange={ onChange }
/>;

<SelectControl
multiple
// @ts-expect-error "string" is not "narrow" or "value"
value={ [ 'string' ] }
options={ [
{
value: 'narrow',
label: 'Narrow',
},
{
value: 'value',
label: 'Value',
},
] }
onChange={ ( value ) =>
// @ts-expect-error "string" is not "narrow" or "value"
value.forEach( ( v ) => v === 'string' )
}
/>;
} );

it( 'should accept an explicit type argument', () => {
<SelectControl< 'narrow' | 'value' >
multiple
// @ts-expect-error "string" is not "narrow" or "value"
value={ [ 'string' ] }
options={ [
{
value: 'narrow',
label: 'Narrow',
},
{
// @ts-expect-error "string" is not "narrow" or "value"
value: 'string',
label: 'String',
},
] }
/>;
} );
} );
} );
/* eslint-enable jest/expect-expect */
} );
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< V 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: V;
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< V extends string = string > =
SelectControlBaseProps< V > & {
/**
* 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< V >;
/**
* 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: NoInfer< V >,
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< V extends string > =
SelectControlBaseProps< V > & {
/**
* 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< V >[];
/**
* 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: NoInfer< V >[],
extra?: { event?: ChangeEvent< HTMLSelectElement > }
) => void;
};

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