Skip to content
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

[Select] onChange parameter has unknown type in Select component #16065

Closed
zhaoyi0113 opened this issue Jun 5, 2019 · 16 comments · Fixed by #16226
Closed

[Select] onChange parameter has unknown type in Select component #16065

zhaoyi0113 opened this issue Jun 5, 2019 · 16 comments · Fixed by #16226
Labels
component: select This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation typescript

Comments

@zhaoyi0113
Copy link

zhaoyi0113 commented Jun 5, 2019

I have the below code to use Select component but I get a type error when upgrading to 4.0.2.

import Select from '@material-ui/core/Select';

<Select native value={selectedValue} onChange={(event) => onChange(event.target.value)}>

This is the error about event.target.value parameter. It says the value is unknown.

Argument of type 'unknown' is not assignable to parameter of type 'string'.
@merceyz
Copy link
Member

merceyz commented Jun 5, 2019

As far as I can tell, you need to specify the type of the value, it's defaulted to unknown.

onChange={(event: React.ChangeEvent<{ value: string }>) => onChange(event.target.value)}

or

onChange={(event) => seValue(event.target.value as string)}

@oliviertassinari Can't find this in the migration guide

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Jun 5, 2019
@eps1lon
Copy link
Member

eps1lon commented Jun 5, 2019

Might be a good idea to include #15245 and #15272 in the migration guide. This particular issue was more of a bug fix. The previous type was too narrow.

@RomWW12
Copy link

RomWW12 commented Jun 5, 2019

In my opinion, unknown is the wrong type for value. As mentioned in TS documentation:

Anything is assignable to unknown, but unknown isn’t assignable to anything.

Which means that in this component, values would accept any type from the outside, but when passing a function to the onChange props like @merceyz we get the following error:

Type 'unknown' is not assignable to type 'string'.ts(2322)

I think the type any is more fit to this situation, do you guys agree ?
If so I would like to open a PR to change this :)

@eps1lon
Copy link
Member

eps1lon commented Jun 5, 2019

If you want opt-out of strict types you can do so by using any. We are striving for strict by default, loose via opt-in. any goes against that.

@zhaoyi0113
Copy link
Author

As far as I can tell, you need to specify the type of the value, it's defaulted to unknown.

onChange={(e: React.ChangeEvent<{ value: string }>) => onChange(e.target.value)}

@oliviertassinari Can't find this in the migration guide

I tried this solution but it doesn't solve the issue. The error I got is:

TS2322: Type '(e: ChangeEvent<{ value: string; }>) => void' is not assignable to type '(event: ChangeEvent<{ name?: string | undefined; value: unknown; }>, child: ReactNode) => void'.
  Types of parameters 'e' and 'event' are incompatible.
    Type 'ChangeEvent<{ name?: string | undefined; value: unknown; }>' is not assignable to type 'ChangeEvent<{ value: string; }>'.
      Type '{ name?: string | undefined; value: unknown; }' is not assignable to type '{ value: string; }'.
        Types of property 'value' are incompatible.
          Type 'unknown' is not assignable to type 'string'.

@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2019

Yes you need to either any it or narrow it at runtime. The value isn't always a string because its type depends on the children. It may be obvious to you but the compiler doesn't understand it (yet).

@zhaoyi0113
Copy link
Author

what is the best option for me to do at the moment? I know I can fix it by set it to any but I don't like any in my code. What do you mean by narrow it at runtime?

@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2019

I know I can fix it by set it to any but I don't like any in my code.

Me neither which is why I changed it to unknown. string was wrong before.

What do you mean by narrow it at runtime?

if (typeof value === 'string') {
  // handle string
} else {
  // something unexpected! throw? warn? ignore?
}

This is what unknown is for. A type safe way of dealing with any value.

@merceyz
Copy link
Member

merceyz commented Jun 6, 2019

@zhaoyi0113 The snippet i provided works perfectly fine for me.

@eps1lon I could make the select generic, so you can provide the type, with it defaulting to unknown

@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2019

@eps1lon I could make the select generic, so you can provide the type, with it defaulting to unknown

Generics in props are very tricky. There was a lot of back and forth with it concerning the component prop which essentially concluded with: TypeScripts support for JSX isn't sufficient.

You can try it but please include some tests with automatic inference and explicit generic argument and interactions between the ´value´ prop type and the the onChange value type.

Even if this all works it's still a hidden as cast. TypeScript cannot check if the value types of the children match the value type in the parent as far as I remember. So all you're doing is making it look sound but it isn't. And while it does create more work for collaborators by having to explain why unknown is the better solution I much prefer this over having an API that isn't safe.

@merceyz
Copy link
Member

merceyz commented Jun 6, 2019

Even if this all works it's still a hidden as cast. TypeScript cannot check if the value types of the children match the value type in the parent as far as I remember. So all you're doing is making it look sound but it isn't

Correct, it can't, however, when the user specifically sets the type, should it not be up to them to make sure it matches?

@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2019

should it not be up to them to make sure it matches?

Then we don't need a type checker or am I misunderstanding something? If they have to make sure that it matches manually why can't they cast to any?

@merceyz
Copy link
Member

merceyz commented Jun 6, 2019

If they have to make sure that it matches manually why can't they cast to any?

I'm not arguing against this, I'm just saying it would be nice to be able to set the type.
If I know my select will only contain string then I would like to set that so the events show the correct type

Or one of the examples from the docs:

<Select<string | number>
  value={state.age}
  onChange={handleChange('age')}
>
  <option value="" />
  <option value={10}>Ten</option>
  <option value={20}>Twenty</option>
  <option value={30}>Thirty</option>
</Select>

@nilamsavani91
Copy link

nilamsavani91 commented Nov 21, 2019

As far as I can tell, you need to specify the type of the value, it's defaulted to unknown.

onChange={(e: React.ChangeEvent<{ value: string }>) => onChange(e.target.value)}

@oliviertassinari Can't find this in the migration guide
@zhaoyi0113 @merceyz I tried as "onChange={(event: React.ChangeEvent<{ value: string }>) => selectGroup({ id: event.target.value })}"

and getting 1 error as below:

Error 1:
No overload matches this call.
Overload 1 of 2, '(props: SelectProps, context?: any): ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<...>)> | Component<...> | null', gave the following error.
Type '(e: ChangeEvent<{ value: string; }>) => void' is not assignable to type '(event: ChangeEvent<{ name?: string | undefined; value: unknown; }>, child: ReactNode) => void'.
Types of parameters 'e' and 'event' are incompatible.
Type 'ChangeEvent<{ name?: string | undefined; value: unknown; }>' is not assignable to type 'ChangeEvent<{ value: string; }>'.
Type '{ name?: string | undefined; value: unknown; }' is not assignable to type '{ value: string; }'.
Types of property 'value' are incompatible.
Type 'unknown' is not assignable to type 'string'.
Overload 2 of 2, '(props: PropsWithChildren, context?: any): ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<...>)> | Component<...> | null', gave the following error.
Type '(e: ChangeEvent<{ value: string; }>) => void' is not assignable to type '(event: ChangeEvent<{ name?: string | undefined; value: unknown; }>, child: ReactNode) => void'.ts(2769)
SelectInput.d.ts(17, 3): The expected type comes from property 'onChange' which is declared here on type 'IntrinsicAttributes & SelectProps'
SelectInput.d.ts(17, 3): The expected type comes from property 'onChange' which is declared here on type 'IntrinsicAttributes & SelectProps & { children?: ReactNode; }'

@alexandermckay
Copy link

alexandermckay commented Aug 19, 2020

onChange={(e) => seValue(e.target.value as string)}

@nahrr

This comment has been minimized.

@oliviertassinari oliviertassinari added the component: select This is the name of the generic UI component, not the React module! label Aug 12, 2021
@oliviertassinari oliviertassinari changed the title onChange parameter has unknown type in Select component [Select] onChange parameter has unknown type in Select component Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation typescript
Projects
None yet
8 participants