SelectControl: improve prop types for single vs multiple selection #47390

ciampo · 2023-01-24T19:12:48Z

What?

Type the props for the SelectControl component with more details, taking advantage of the fact that that the value and onChange prop types are affected by whether the component has multiple selection enabled or not

Why?

The way SelectControl is currently typed is not precise and can lead to hacky types workarounds, like in #46721

How?

Rewrite SelectControlProps by making the result of the union of two types:

when multiple is true, we can assume that value (and therefore the argument of onChange) is of type string[]
otherwise, we can assume that value (and the onChange argument) is of type string

Testing Instructions

Review types
Make sure that SelectControl works as expected in Storybook, and that Storybook docs still look correct
Make sure that the component behaves as expected across the editor (i.e. in QueryControls when editing the settings of the "Latest posts" block)

ciampo · 2023-03-02T13:29:24Z

Hey @sirreal , I'd love to get your help on this (although it's definitely not urgent!).

What I'd like to land on, is a way for SelectControl to automatically infer whether the multiple prop is true — in that case, it should expect a value of type string[]. Otherwise, if the multiple prop is not specified (or false), the value prop should be of type string.

The onChange property should also change (its first argument should be typeof value, whatever type value is).

Is there a way to write types so that a consumer of SelectControl gets the correct narrowed types automatically? ie.

<SelectControl 
  multiple={ true }
  // This should error, but it's currently ok because at the moment `value` can be both `string` or `string[]`
  value="example"
/>

Thank you!

sirreal · 2023-03-02T17:23:54Z

👋 I took a look and have an example implementation to share.

Whenever we say "this needs to be this or that" we're talking about a union type. In this case we have multiple as a discriminator, giving us a discriminated union. That means that we can be one type or another type depending on multiple. This should give us all the ingredients necessary to implement this 😁

These are the important bits. We make the props we want for either side of the union, one with multiple: true and the other with an optional multiple?: false (undefined or omission also serve to discriminate to this side of the union). These two interfaces contain the types that change depending on multiple.

For convenience, we put the shared props in another interface and each side of the union extends that.

interface PropsBase {
  readonly aNumber: number;
  readonly aString: string;
}

interface PropsMultiple extends PropsBase {
  readonly multiple: true;
  readonly value: ReadonlyArray<string>;
  readonly onChange: (values: ReadonlyArray<string>) => void;
}

interface PropsSingle extends PropsBase {
  readonly multiple?: false;
  readonly value: string;
  readonly onChange: (values: string) => void;
}

type Props = PropsMultiple | PropsSingle;

Then, when we go to use the component, based on the presence or absence (truthiness) of multiple, we get the results we're looking for:

import type { FC } from 'react';

declare const Komponent: FC<Props>;

<Komponent aNumber={0} aString='a' value='a' onChange={(val:string) => val.toLowerCase()}/>;
// @ts-expect-error This has multiple but is a single string!
<Komponent aNumber={0} aString='a' multiple value='a' onChange={(val:string) => val.toLowerCase()}/>;

<Komponent aNumber={0} aString='a' multiple value={['a']} onChange={(val:readonly string[]) => val.map(s=>s.toLowerCase())}/>;
// @ts-expect-error This is an array of strings but doesn't have multiple!
<Komponent aNumber={0} aString='a'  value={['a']} onChange={(val:readonly string[]) => val.map(s=>s.toLowerCase())}/>;

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

ciampo · 2023-03-06T17:49:50Z

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

+const SelectControlWithState: ComponentStory< typeof SelectControl > = (
+	props
+) => {
+	const [ selection, setSelection ] = useState< string[] >();
+
+	if ( props.multiple ) {


This felt slightly hacky, but I couldn't figure out a much more elegant way.

Basically, since the value of SelectControl is controlled with the selection variable from internal state, I couldn't find an easy way to get TypeScript to automatically understand when selection can be used as a string and when it can be used as a string[].

Therefore, I decided to have selection to always be string[], and I've added some code to "adapt" the array to a single value for when multiple !== true.

In doing do, I also had to "override" the multiple prop passed to SelectControl to avoid a TypeScript error.

For stories this seems reasonable.

Agreed with Jon here, I believe it also makes it quite explicit and transparent what we're aiming to do, which is always useful for a story/test/example.

ciampo · 2023-03-06T17:53:10Z

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

+// `TreeSelect` inherits props from `SelectControl`, but only
+// in single selection mode (ie. when the `multiple` prop is not defined).
+export interface TreeSelectProps
+	extends Omit< SelectControlSingleSelectionProps, 'value' | 'multiple' > {


We can omit the multiple prop from TreeSelect (since it doesn't seem to be relevant to the component). This simplified the component's prop types and removes a bunch of TS errors, allowing us to remove hacks like the ones in the QueryControls component

ciampo · 2023-03-06T17:54:50Z

Hey @sirreal , I pushed some changes to the component. The types should now work as expected, but it would be great to get some final feedback from you before I mark the PR as ready for final review.

I added some self-review comments to point out the most interesting aspects of the PR.

github-actions · 2023-03-06T18:16:54Z

Flaky tests detected in ebe1e8f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4354320245
📝 Reported issues:

[Flaky Test] should filter results properly #44462 in specs/site-editor/global-styles-sidebar.test.js
[Flaky Test] should navigate block hierarchy using only the keyboard #47640 in specs/editor/various/block-hierarchy-navigation.test.js

sirreal

I looked this over and the changes seem reasonable to me 👍

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

sirreal · 2023-03-07T09:45:05Z

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

+const SelectControlWithState: ComponentStory< typeof SelectControl > = (
+	props
+) => {
+	const [ selection, setSelection ] = useState< string[] >();
+
+	if ( props.multiple ) {


For stories this seems reasonable.

ciampo · 2023-03-07T10:07:17Z

After solving a couple of TypeScript hurdles, this PR is now ready for review

tyxla

Looks great 👍 Nice work unionizing the single and multiple selection scenarios 🚀

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

tyxla · 2023-03-07T10:53:41Z

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

@@ -39,7 +41,7 @@ function UnforwardedSelectControl(
 		label,
 		multiple = false,
 		onBlur = noop,
-		onChange = noop,
+		onChange,


Breaking change?

It shouldn't be, since I've added optional chaining when calling it in the file, which should have the same effect as defaulting to noop

tyxla · 2023-03-07T10:55:27Z

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

+const SelectControlWithState: ComponentStory< typeof SelectControl > = (
+	props
+) => {
+	const [ selection, setSelection ] = useState< string[] >();
+
+	if ( props.multiple ) {


Agreed with Jon here, I believe it also makes it quite explicit and transparent what we're aiming to do, which is always useful for a story/test/example.

ciampo · 2023-03-07T12:54:17Z

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

 	};

 	const classes = classNames( 'components-select-control', className );

-	/* eslint-disable jsx-a11y/no-onchange */


I realised that this rule was not relevant anymore — removing this line doesn't seem to cause any ESLint errors. Furthermore, the rule is deprecated

bph · 2023-04-04T13:41:58Z

I am not certain that there is actually an API change or if this just converted the component to typescript to improve the DX experience. Flagged for Dev note to get a mention in the Misc Editor Dev note for 6.3

ciampo · 2023-04-04T15:54:49Z

Hey @bph , this PR only improves the types for the SelectControl props, no API changes were made. I don't think we need a dev note for this.

bph · 2023-04-04T16:23:33Z

Thank you.

ciampo changed the title ~~SelectControl: improve value types~~ SelectControl: improve value and onChange prop types Jan 24, 2023

ciampo mentioned this pull request Jan 27, 2023

DimensionControl(Experimental): refactor to TypeScript #47351

Merged

ciampo force-pushed the fix/select-control-types branch from 6907390 to 43efc9f Compare March 2, 2023 15:05

ciampo force-pushed the fix/select-control-types branch from 43efc9f to bdcf2a5 Compare March 6, 2023 17:40

ciampo self-assigned this Mar 6, 2023

ciampo force-pushed the fix/select-control-types branch from bdcf2a5 to bef66de Compare March 6, 2023 17:43

ciampo commented Mar 6, 2023

View reviewed changes

packages/components/src/select-control/index.tsx Outdated Show resolved Hide resolved

ciampo commented Mar 6, 2023

View reviewed changes

ciampo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Mar 6, 2023

ciampo changed the title ~~SelectControl: improve value and onChange prop types~~ SelectControl: improve props types for single vs multiple selection Mar 6, 2023

ciampo changed the title ~~SelectControl: improve props types for single vs multiple selection~~ SelectControl: improve prop types for single vs multiple selection Mar 6, 2023

sirreal reviewed Mar 7, 2023

View reviewed changes

ciampo added 9 commits March 7, 2023 10:58

Rework types, split into multiple vs single selection

16ace46

Rework Storybook example

afe1a3c

Remove redundant onChange type

a48ab74

Adding logic for single vs multiple selection to remove type errors

7beaf5d

TreeSelect: exclude multiple prop, fix type errors

5e8f768

Remove unnecessary type hacks around TreeSelect and the onChange prop

531a659

Replace type guard with simple prop check

3d7f837

Pass event object for both multiple and single value selection

c348564

CHANGELOG

4a7c322

ciampo force-pushed the fix/select-control-types branch from bef66de to 4a7c322 Compare March 7, 2023 10:03

ciampo marked this pull request as ready for review March 7, 2023 10:05

ciampo requested a review from ajitbohra as a code owner March 7, 2023 10:05

ciampo requested review from tyxla, mirka, kienstra, torounit and chad1008 March 7, 2023 10:06

tyxla approved these changes Mar 7, 2023

View reviewed changes

ciampo added 2 commits March 7, 2023 13:41

Update types

7b9fda1

Remove unnecessary eslint ignore rule

ebe1e8f

ciampo commented Mar 7, 2023

View reviewed changes

ciampo enabled auto-merge (squash) March 7, 2023 12:54

ciampo merged commit 583d748 into trunk Mar 7, 2023

ciampo deleted the fix/select-control-types branch March 7, 2023 13:22

github-actions bot added this to the Gutenberg 15.4 milestone Mar 7, 2023

ciampo mentioned this pull request Mar 9, 2023

CustomGradientPicker: Refactor to TypeScript #48929

Merged

bph added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 4, 2023

bph removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 4, 2023

ciampo mentioned this pull request Sep 17, 2023

Refactor @wordpress/components package to TypeScript #35744

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SelectControl: improve prop types for single vs multiple selection #47390

SelectControl: improve prop types for single vs multiple selection #47390

ciampo commented Jan 24, 2023 •

edited

Loading

ciampo commented Mar 2, 2023

sirreal commented Mar 2, 2023

ciampo Mar 6, 2023

sirreal Mar 7, 2023

tyxla Mar 7, 2023

ciampo Mar 6, 2023

ciampo commented Mar 6, 2023

github-actions bot commented Mar 6, 2023 •

edited

Loading

sirreal left a comment

sirreal Mar 7, 2023

ciampo commented Mar 7, 2023

tyxla left a comment

tyxla Mar 7, 2023

ciampo Mar 7, 2023

tyxla Mar 7, 2023

ciampo Mar 7, 2023

bph commented Apr 4, 2023 •

edited

Loading

ciampo commented Apr 4, 2023

bph commented Apr 4, 2023

SelectControl: improve prop types for single vs multiple selection #47390

SelectControl: improve prop types for single vs multiple selection #47390

Conversation

ciampo commented Jan 24, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

ciampo commented Mar 2, 2023

sirreal commented Mar 2, 2023

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

ciampo commented Mar 6, 2023

github-actions bot commented Mar 6, 2023 • edited Loading

sirreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo commented Mar 7, 2023

tyxla left a 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

bph commented Apr 4, 2023 • edited Loading

ciampo commented Apr 4, 2023

bph commented Apr 4, 2023

ciampo commented Jan 24, 2023 •

edited

Loading

github-actions bot commented Mar 6, 2023 •

edited

Loading

bph commented Apr 4, 2023 •

edited

Loading