CustomSelectControl: improve props type inferring #64412
Conversation
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
akasunil
added
[Type] Bug
| const onChange: ( | ||
| changeObject: CustomSelectChangeObject< CustomSelectOption > | ||
| ) => void = () => {}; | ||
| const onChangeWithFoo: ( | ||
| changeObject: CustomSelectChangeObject< | ||
| CustomSelectOption & { | ||
| foo: string; | ||
| } | ||
| > | ||
| ) => void = () => {}; |
There was a problem hiding this comment.
Ideally we would write these tests without having to import the CustomSelectChangeObject and CustomSelectOption types directly. Consumers outside of this package don't have access to these internal types.
| const onChange: ( | |
| changeObject: CustomSelectChangeObject< CustomSelectOption > | |
| ) => void = () => {}; | |
| const onChangeWithFoo: ( | |
| changeObject: CustomSelectChangeObject< | |
| CustomSelectOption & { | |
| foo: string; | |
| } | |
| > | |
| ) => void = () => {}; | |
| const onChange: React.ComponentProps< | |
| typeof UncontrolledCustomSelectControl | |
| >[ 'onChange' ] = () => {}; |
And for the onChangeWithFoo cases, I think we could simplify it into a more realistic example and just inline it:
<UncontrolledCustomSelectControl
label="Label"
options={ options }
value={ {
key: 'narrow',
name: 'Narrow',
} }
// @ts-expect-error: the option type should not be inferred from `onChange`
onChange={ ( obj ) => obj.foo }
/>;There was a problem hiding this comment.
Updated.
As for the onChangeWithFoo cases, what you provided above should not be what we want.
We need to test if the NoInfer in the onChange method works as expected, so we have to define the type of the onChange prop explicitly.
It should not be a realistic use case for sure, but we still need to test it. So I added some comments to explain it.
There was a problem hiding this comment.
Ah, you're right. Thanks for the simplification and code comment though, it reads a lot better 👍
| @@ -24,7 +24,7 @@ export type CustomSelectOption = { | |||
| /** | |||
| * The object returned from the onChange event. | |||
| */ | |||
| type CustomSelectChangeObject< T extends CustomSelectOption > = { | |||
| export type CustomSelectChangeObject< T extends CustomSelectOption > = { | |||
There was a problem hiding this comment.
Better if we could remove this export, as noted above.
…ect-control-type-improve
| @@ -689,3 +689,128 @@ describe.each( [ | |||
| } ); | |||
| } ); | |||
| } ); | |||
|
|
|||
| describe( 'Type checking', () => { | |||
There was a problem hiding this comment.
Not something for this PR, but I wonder if we should start putting these type-checking pseudo-tests in a separate file under /test (types.tsx for example).
Then we could add a global ESLint disable rule for jest/expect-expect and we'll no longer have to do it every time.
Thoughts?
What?
Improve props type inferring for
CustomSelectControlcomponent.Why?
The current implementation (introduced in #63985) has some drawbacks:
valueandonChange, which is not expected.optionsdoes not accept immutable array.How?
Followed the implementation of #64069 :
NoInferReadonlyArrayforoptionscc @mirka
Testing Instructions
Added type checking in unit tests
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A