SelectControl: Infer `value` type from `options` #64069

mirka · 2024-07-29T19:30:05Z

Updated take on #60293 (props @mikeybinns)

What?

Tighten the TypeScript type checking on SelectControl, so the value type matches in the value, options, and onChange props.

See Dev Notes at the bottom for how this looks like to a consumer.

Why?

It can catch potential bugs.

How?

Adding a generic that can either be inferred from the options array (if it is immutable), or be set explicitly.

Testing Instructions

See some of the static type tests in the unit test file, and play around by adding your own cases and see what errors.

✍️ Dev Note

Stricter type checking in `SelectControl`

In TypeScript codebases, SelectControl will now be able to type check whether the value type matches in the value, options, and onChange props.

This will be inferred automatically if the options array is declared as immutable.

<SelectControl
	// @ts-expect-error "baz" is not in `options` array
	value="baz"
	options={ [ // array literal that cannot be mutated
		{
			value: 'foo',
			label: 'Foo',
		},
		{
			value: 'bar',
			label: 'Bar',
		},
	] }
/>;

const options = [
	{
		value: 'foo',
		label: 'Foo',
	},
	{
		value: 'bar',
		label: 'Bar',
	},
] as const; // const assertion to mark as readonly

<SelectControl
	// @ts-expect-error "baz" is not in `options` array
	value="baz"
	options={ options }
/>;

If the options array is mutable, value will remain a simple string type.

This is effectively the same type checking behavior as before.

const options = [
	{
		value: 'foo',
		label: 'Foo',
	},
	{
		value: 'bar',
		label: 'Bar',
	},
];

<SelectControl
	// No error, because `value` has type `string`
	value="baz"
	options={ options }
/>;

You can also set an explicit value type.

<SelectControl< 'foo' | 'bar' >
	// @ts-expect-error "baz" is not in `'foo' | 'bar'`
	value="baz"
	// @ts-expect-error "baz" is not in `'foo' | 'bar'`
	options={ [ { value: 'baz', label: 'Baz' } ] }
/>;

mirka · 2024-07-29T19:55:28Z

packages/components/src/date-time/time/index.tsx

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 😅)

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?

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).

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.

mirka · 2024-07-29T19:56:15Z

packages/components/src/query-controls/index.tsx

+						value={
+							orderBy === undefined || order === undefined
+								? undefined
+								: `${ orderBy }/${ order }`
+						}


This was an actual bug.

mirka · 2024-07-29T19:57:58Z

packages/components/src/select-control/index.tsx

+			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 } );


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

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

If you edit a website with dev tools, it's your fault you broke the code, not the code maintainer

The likelihood of this happening is next to none

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'.

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.

mirka · 2024-07-29T19:59:38Z

packages/components/src/select-control/types.ts

@@ -24,7 +24,7 @@ type SelectControlBaseProps = Pick<
 	Pick< BaseControlProps, 'help' | '__nextHasNoMarginBottom' > & {
 		onBlur?: ( event: FocusEvent< HTMLSelectElement > ) => void;
 		onFocus?: ( event: FocusEvent< HTMLSelectElement > ) => void;
-		options?: {
+		options?: readonly {


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

mirka · 2024-07-29T20:08:14Z

packages/components/src/select-control/types.ts

+		 * @default false
+		 */
+		multiple?: false;
+		value?: NoInfer< T >;


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+.

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.

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 🙂

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.

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

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

mikeybinns

I have some personal nit-picks, but ultimately approve it because I'd be fine if those were ignored :)

mikeybinns · 2024-07-29T20:34:03Z

packages/components/src/select-control/index.tsx

+			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 } );


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

If you edit a website with dev tools, it's your fault you broke the code, not the code maintainer

The likelihood of this happening is next to none

mikeybinns · 2024-07-29T20:37:51Z

packages/components/src/select-control/index.tsx

+			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 } );


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'.

mikeybinns · 2024-07-29T20:42:47Z

One thing that's missing from my PR is the automated tests, perhaps it's worth copying those over?

ciampo · 2024-07-30T07:18:13Z

packages/components/src/select-control/types.ts

+		 * Otherwise, the value received is a single value with the new selected value.
+		 */
+		onChange?: (
+			value: T,


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

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

ciampo · 2024-07-30T07:24:34Z

packages/components/src/date-time/time/index.tsx

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).

Co-authored-by: Mikey Binns <hello@mikeybinns.com>

mirka · 2024-08-01T19:20:20Z

One thing that's missing from my PR is the automated tests, perhaps it's worth copying those over?

You're right, I brought over your tests and reformatted them a bit to mesh with our existing type tests (ad20004). I tried to pare it down so we have a good cost/benefit balance between safety and maintenance burden — hope that's reasonable.

github-actions · 2024-08-01T19:26:56Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: mikeybinns <mikeybinns@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

mirka · 2024-08-01T19:35:21Z

I discussed this with a few other folks and nobody is outright opposed, so I think we can move forward to an official review so this can be merged 🙏

DaniGuardiola

Good stuff! Straightforward implementation. Nice touch with the type tests, though it makes me wonder if we should add standards for type testing and/or support for tooling like https://github.com/MichiganTypeScript/type-testing - the eslint ignore comments for jest feel a bit like a hack.

As a reference, here's an example from a personal project where I do extensive type tests: https://github.com/DaniGuardiola/rpc-anywhere/blob/main/src/tests/types-test.ts

Other than that, I have no notes (other than a minor nit)!

DaniGuardiola · 2024-08-06T17:48:09Z

packages/components/src/select-control/index.tsx

+		'select',
+		false
+	> & { ref?: React.Ref< HTMLSelectElement > }
+) => ReturnType< typeof UnforwardedSelectControl >;


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.

That makes sense, thanks 🙏 2f195c2

# Conflicts: # packages/components/CHANGELOG.md # packages/components/src/select-control/index.tsx # packages/components/src/select-control/test/select-control.tsx # packages/components/src/select-control/types.ts

mirka · 2024-08-06T21:45:42Z

Nice touch with the type tests, though it makes me wonder if we should add standards for type testing and/or support for tooling like https://github.com/MichiganTypeScript/type-testing - the eslint ignore comments for jest feel a bit like a hack.

@DaniGuardiola It has come up, most recently in #62400 (comment). I don't think we're quite at the point where we need dedicated type test tooling, as the downsides are still outweighing the upsides in my opinion (e.g. having to manage the tooling package updates, forcing contributors to learn yet another testing library, etc).

But I do think we should start to document some standards/conventions like you suggest, as this is the fourth time we're doing static type tests here!

mikeybinns · 2024-08-06T23:16:48Z

Thanks for all your work on this @mirka ☺️

mirka · 2024-08-07T10:29:32Z

@mikeybinns Thanks for all your work as well 🙌

SelectControl: Infer value type from options

2ca2f43

mirka added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jul 29, 2024

mirka self-assigned this Jul 29, 2024

mirka commented Jul 29, 2024

View reviewed changes

mirka mentioned this pull request Jul 29, 2024

Improve select control typescript types #60293

Closed

mikeybinns approved these changes Jul 29, 2024

View reviewed changes

ciampo reviewed Jul 30, 2024

View reviewed changes

mirka mentioned this pull request Jul 31, 2024

Button: Fix tertiary destructive hover style #64152

Merged

mirka and others added 3 commits August 2, 2024 03:24

Don't infer onChange

34ca3a0

Add static type tests

ad20004

Co-authored-by: Mikey Binns <hello@mikeybinns.com>

Rename generic to V (for value)

2ff5b37

mirka added the has dev note when dev note is done (for upcoming WordPress release) label Aug 1, 2024

mirka added 2 commits August 2, 2024 04:21

Merge branch 'trunk' into infer-select-control-types

ee8e353

Add changelog

0770b14

mirka marked this pull request as ready for review August 1, 2024 19:26

mirka requested a review from ajitbohra as a code owner August 1, 2024 19:26

tyxla requested a review from a team August 2, 2024 16:22

DaniGuardiola approved these changes Aug 6, 2024

View reviewed changes

mirka added 2 commits August 7, 2024 06:16

Simplify return type

2f195c2

Merge branch 'trunk' into infer-select-control-types

7488d9a

# Conflicts: # packages/components/CHANGELOG.md # packages/components/src/select-control/index.tsx # packages/components/src/select-control/test/select-control.tsx # packages/components/src/select-control/types.ts

mirka enabled auto-merge (squash) August 6, 2024 21:46

mirka merged commit de33f91 into trunk Aug 6, 2024
61 checks passed

mirka deleted the infer-select-control-types branch August 6, 2024 22:08

github-actions bot added this to the Gutenberg 19.0 milestone Aug 6, 2024

This was referenced Aug 10, 2024

TextControl: support generic props type #64293

Open

CustomSelectControl: improve props type inferring #64412

Merged

fabiankaegy mentioned this pull request Oct 1, 2024

6.7 Dev Note Tracking Issue #65784

Closed

97 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SelectControl: Infer `value` type from `options` #64069

SelectControl: Infer `value` type from `options` #64069

mirka commented Jul 29, 2024 •

edited

Loading

mirka Jul 29, 2024

mikeybinns Jul 29, 2024

ciampo Jul 30, 2024

mirka Aug 1, 2024

mirka Jul 29, 2024

mirka Jul 29, 2024

mikeybinns Jul 29, 2024

mikeybinns Jul 29, 2024

mirka Aug 1, 2024

mirka Jul 29, 2024

mirka Jul 29, 2024

jsnajdr Aug 2, 2024

mikeybinns Aug 2, 2024

jsnajdr Aug 4, 2024

tyxla Aug 5, 2024

meteorlxy Aug 10, 2024

mikeybinns left a comment

mikeybinns Jul 29, 2024

mikeybinns Jul 29, 2024

mikeybinns commented Jul 29, 2024

ciampo Jul 30, 2024

mirka Aug 1, 2024

ciampo Jul 30, 2024

mirka commented Aug 1, 2024

github-actions bot commented Aug 1, 2024 •

edited

Loading

mirka commented Aug 1, 2024

DaniGuardiola left a comment

DaniGuardiola Aug 6, 2024

mirka Aug 6, 2024

mirka commented Aug 6, 2024

mikeybinns commented Aug 6, 2024

mirka commented Aug 7, 2024

SelectControl: Infer value type from options #64069

SelectControl: Infer value type from options #64069

Conversation

mirka commented Jul 29, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

✍️ Dev Note

Stricter type checking in SelectControl

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeybinns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeybinns commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka commented Aug 1, 2024

github-actions bot commented Aug 1, 2024 • edited Loading

mirka commented Aug 1, 2024

DaniGuardiola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka commented Aug 6, 2024

mikeybinns commented Aug 6, 2024

mirka commented Aug 7, 2024

SelectControl: Infer `value` type from `options` #64069

SelectControl: Infer `value` type from `options` #64069

mirka commented Jul 29, 2024 •

edited

Loading

Stricter type checking in `SelectControl`

github-actions bot commented Aug 1, 2024 •

edited

Loading