Improve select control typescript types #60293

mikeybinns · 2024-03-28T21:00:38Z

What?

Allow optional narrowing of the SelectControl component for better type validation.

Why?

This is an enhancement to the type of the SelectControl. It will help users of Typescript narrow down their types to prevent errors. It also infers types if options is provided which may flag code errors with some implementations. There is no change to the base JS code, this is only a typescript change.
This generic type was previously available in the 3rd party types package, but was removed when components moved to 1st party types.

Fixes #16060

How?

I have made the type for the SelectControl component generic, which allows users to explicitly provide an option type, or infers that type from the provided options array.
If you have provided an options array but don't want the narrower type, you can specify <string> as the generic to re-loosen this type.

In adding this enhancement, there are two implementations with @wordpress/components which needed to be updated.

TimePicker in date-time/time which was using a select for the month. I have added the necessary type to tighten the type here.
QueryControls in query-controls which actually had a bug in its implementation. The undefined states were not being accounted for in the string interpolation, leading potentially to undefined/undefined as the value. This would not affect runtime as the browser knows that option doesn't exist and defaults to the first option, but I have fixed the bug to satisfy typescript.

Testing Instructions

I have included a file to add type-related tests, and added each possible test case. For testing, see that file (select-control/types-test.tsx). Future changes which break implementation would throw a type error in that file, preventing linting from passing.

Implementation examples taken from `types-test.tsx`

// Normal select, no generic provided. Value type is inferred from available options.
// Incorrect value type provided to test validation is working.
<SelectControl
  // @ts-expect-error "string" is not "narrow" or "value", so throw an error
  value="string"
  multiple={ false }
  options={ [
    {
      value: 'narrow',
      label: 'Narrow',
    },
    {
      value: 'value',
      label: 'Value',
    },
  ] }
  onChange={ ( value ) => {
    // Typescript can't guarantee the value isn't modified by the
    // user, so we can't assign a type to value here.
    return value === 'string';
  } }
/>;

// Select with explicit generic provided.
// Both value and options.value must match the provided generic.
// Incorrect value type provided to test validation is working.
<SelectControl< 'narrow' | 'value' >
  // @ts-expect-error "string" is not "narrow" or "value", so throw an error
  value="string"
  multiple={ false }
  options={ [
    {
      // @ts-expect-error "string" is not "narrow" or "value", so throw an error
      value: 'string',
      label: 'String',
    },
    {
      // @ts-expect-error "otherstring" is not "narrow" or "value", so throw an error
      value: 'otherstring',
      label: 'Other String',
    },
  ] }
/>;

// Select with explicit generic provided.
// Usually this would result in a type error with no generic,
// but this has been explicitly loosened to bypass errors.
<SelectControl< string >
  value="string"
  multiple={ false }
  options={ [
    {
      value: 'narrow',
      label: 'Narrow',
    },
    {
      value: 'value',
      label: 'Value',
    },
  ] }
/>;

github-actions · 2024-03-28T21:00:54Z

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @AlchemyUnited.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

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

Unlinked contributors: AlchemyUnited.

Co-authored-by: mikeybinns <mikeybinns@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

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

github-actions · 2024-03-28T21:01:06Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @mikeybinns! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

mikeybinns · 2024-04-08T17:28:03Z

@mirka any chance you can take a look at this? I'd love to get this added if possible :)

mirka

This is an excellent PR with a great description and test coverage (I've been starting to do static TS tests as well, so happy to see someone doing similar 😄), thank you!

That said, could you help me understand the use case for this? The types can already be accessed and be reused or overridden in any way, for example:

type SelectControlOption = NonNullable<
	React.ComponentProps< typeof SelectControl >[ 'options' ]
>[ number ];
type MyNarrowOption = Omit< SelectControlOption, 'value' > & {
	value: 'foo' | 'bar';
};
type MyNarrowSelectControlProps = Omit<
	React.ComponentProps< typeof SelectControl >,
	'options'
> & { options?: MyNarrowOption[] };

or then type check it somewhere in your consumer code:

const options: MyNarrowOption[] = [
	{
		label: 'Foo',
		value: 'foo',
	},
];
const myDefaultValue: MyNarrowOption[ 'value' ] = 'foo';

<SelectControl value={ myDefaultValue } options={ options } />;

Is it mostly about being able to do this in a more ergonomic way? If it's about ergonomics, my concern is that there are a ton of ways a consumer might want to narrow down some aspect of a component type, and it's not sustainable to cover every one of those possibilities by baking it into the wp-component as a generic. It might make sense when there is a very common use case, but this one for SelectControl seems really niche to me.

On the other hand, if there are good reasons other than ergonomics, then we might want to consider addressing this in a slightly more abstracted way, as it will likely be relevant to our other components as well.

Let me know what you think!

mikeybinns · 2024-04-11T20:16:55Z

This is an excellent PR

Thanks that's always good to hear 😄

So, yes, I will admit, this is mostly about ergonomics. The type library on DT used to have this generic before it was moved over to 1st party types and I really missed being able to quickly narrow down the type.

With that being said, one of the major benefits of using a generic over other ways is that they can be automatically inferred by TypeScript. Take this example:

<SelectControl
  value="string"
  options={ [
    {
      value: 'narrow',
      label: 'Narrow',
    },
    {
      value: 'value',
      label: 'Value',
    },
  ] }
/>;

TypeScript can automatically infer from the generic that the only options should be "narrow" and "value", so "string" is invalid. The end user has just caught a bug without having to add any extra code on their end, and they may have not even realised it was a bug until TypeScript warns them about it.

It think because of this automatic inference, it's worth adding, and that the ability for users to add value types manually is just a side benefit.

mikeybinns · 2024-04-11T20:30:08Z

I'll also note that the QueryControls bug I found is a good real world example of why this is useful, even if the bug was minor in severity.

But to play devils advocate against myself, the TimePicker "bug" I found was not really a bug in the code, just a typescript loose type and may produce errors for users if the type of value is looser than the type of the generic. 🤷

AlchemyUnited · 2024-04-11T21:15:06Z

@mikeybinns - Thanks for taking care of this.

mirka · 2024-04-16T23:32:57Z

Thank you for clarifying, that was very helpful! Just wanted to let you know that this is on my todo list, as I want to think about it a bit more and take into consideration some other components that might benefit from this kind of type checking. I appreciate your patience 🙏

mirka · 2024-07-29T20:18:07Z

Apologies for the long delay! This became outdated due to TypeScript version updates (5.1 to 5.4), so I went ahead and did a 5.4-compliant version at #64069, simplifying things in a few spots as well. It would be great if you could take a look and let me know what you think.

mikeybinns · 2024-08-04T22:18:44Z

Superseded by #64069

mikeybinns added 4 commits March 28, 2024 20:30

improve select control typescript types

ff07e14

remove repeated onChange in tests

dd211b8

add test for explicitly loosened type

96590ad

fix error comment references

a14fa02

mikeybinns requested a review from ajitbohra as a code owner March 28, 2024 21:00

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 28, 2024

mikeybinns added 2 commits March 28, 2024 21:19

move types test file out of test folder so jest ignores it

2e6ce1d

add changelog entry

c050bc2

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

mirka assigned mikeybinns Apr 8, 2024

mirka self-requested a review April 8, 2024 17:30

mirka reviewed Apr 11, 2024

View reviewed changes

This was referenced Jul 29, 2024

CustomSelectControl: Support generic props type #63984

Closed

SelectControl: Infer value type from options #64069

Merged

mikeybinns closed this Aug 4, 2024

mikeybinns deleted the improve-select-control-types branch August 4, 2024 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve select control typescript types #60293

Improve select control typescript types #60293

Uh oh!

mikeybinns commented Mar 28, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Mar 28, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Mar 28, 2024

Uh oh!

mikeybinns commented Apr 8, 2024

Uh oh!

mirka left a comment

Uh oh!

mikeybinns commented Apr 11, 2024 •

edited

Loading

Uh oh!

mikeybinns commented Apr 11, 2024

Uh oh!

AlchemyUnited commented Apr 11, 2024

Uh oh!

mirka commented Apr 16, 2024

Uh oh!

mirka commented Jul 29, 2024

Uh oh!

mikeybinns commented Aug 4, 2024 •

edited

Loading

Uh oh!

Uh oh!

Improve select control typescript types #60293

Improve select control typescript types #60293

Uh oh!

Conversation

mikeybinns commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

How?

Testing Instructions

Implementation examples taken from types-test.tsx

Uh oh!

github-actions bot commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unlinked Accounts

Uh oh!

github-actions bot commented Mar 28, 2024

Uh oh!

mikeybinns commented Apr 8, 2024

Uh oh!

mirka left a comment

Choose a reason for hiding this comment

Uh oh!

mikeybinns commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeybinns commented Apr 11, 2024

Uh oh!

AlchemyUnited commented Apr 11, 2024

Uh oh!

mirka commented Apr 16, 2024

Uh oh!

mirka commented Jul 29, 2024

Uh oh!

mikeybinns commented Aug 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mikeybinns commented Mar 28, 2024 •

edited

Loading

Implementation examples taken from `types-test.tsx`

github-actions bot commented Mar 28, 2024 •

edited

Loading

mikeybinns commented Apr 11, 2024 •

edited

Loading

mikeybinns commented Aug 4, 2024 •

edited

Loading