-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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.
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
[Autocomplete] Improve typings #18854
[Autocomplete] Improve typings #18854
Conversation
No bundle size changes comparing e5b2e22...f66e47b |
@testarossaaaaa How is the exploration of the problem going? |
@oliviertassinari on vacation now, on the 10th I can finish it. Types can already be used, if I'm not mistaken, they cover 80% of cases. As I see there was an error on CI side. |
@testarossaaaaa Awesome, I have looked a bit at the test fails, it should be green. However, I have noticed this new message during the prop-types generation:
|
@testarossaaaaa Alright, I have tried to clean up the demos, it would be awesome if you could double-check :). |
@@ -62,7 +62,7 @@ export default function Playground() { | |||
{...defaultProps} | |||
id="controlled-demo" | |||
value={value} | |||
onChange={(event, newValue) => { | |||
onChange={(event: any, newValue: FilmOptionType | null) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript can't discriminate union type without specifying the multiple prop, it's why we need to write concrete type here, an another solution for typescript users is settingmultiple={false}
or multiple={undefined}
so typescript can infer right typings for onChange handler, what way do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a bug in typescript microsoft/TypeScript#35769
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to know. Should we specify the multiple prop (which will be closer to real life usage)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can explicitly write multiple={undefined}
in typescript examples (with a mention in the documentation why this is needed), and when the bug is fixed, this will not be a breaking change. Or strictly require to specify multiple
as true
or false
, but it seems that this is not a great DX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, it's only needed because we spread {...defaultProps}
? What if we duplicate the props instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks, in this can, I think that it would be better not to change the current tradeoff. The usage of the TypeScript demo is at 10% (and growing). It would look weird to see undefined. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a typescript developer has not defined the multiple prop as false or undefined than all other dependent props will also receive invalid typings, not only the onChange prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about we document it in the ## Limitations
section of the page for TypeScript users, with a link to the related TypeScript issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. So we explicitly provide typings for onChange methods, like in this line of code above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be my proposal, I believe we already get significant improvements with your changes. This is a roadblock that will require more effort.
Thank you for pushing this further, I hope you had great holidays :). |
I believe this was the change that is causing an issue for my project. thoughts?
I upgraded from 4.0.0-alpha.39 to 4.0.0-alpha.42, and it started saying I need a type parameter. ok, cool. for the time being, I tried
but then I get in a place where we're using the component:
and says Type '{ autofocus: true; options: { key: number; value: IMetroArea; label: string; }[]; getOptionLabel: (option: Option) => string; onChange: (changeEvent: ChangeEvent<{}>, selectedCityOption: { key: string; label: string; value: MetroArea; } | null) => void; onClose: () => void; }' is not assignable to type 'IntrinsicAttributes & AutocompleteProps'. and in a different spot:
Type '{ label: string; classes: { noOptions: string | undefined; }; options: SelectOption[] | undefined; getOptionLabel: (option: Option) => string; defaultValue: { key: string; value: string; label: string; } | undefined; TextFieldProps: { value: string | undefined; onChange: (searchChange: ChangeEvent) => void; }; onChange: (event: ChangeEvent<{}>, value: Option | null) => void; onClose: () => void; }' is not assignable to type 'IntrinsicAttributes & AutocompleteProps'. any ideas on this? what's interesting to me is :
where those 2 are in the removals part of the standard props interface. |
Closes #18810