[Select] Fix incorrect event.target type in onChange #15272
Conversation
|
No bundle size changes comparing 1d6425d...5c04e3c |
eps1lon
changed the title
If this is actually not safe then the error originates from the write. We want casts (unsafe) where we might need to add sanitization/validation
| @@ -84,7 +86,7 @@ class MaxWidthDialog extends React.Component { | |||
| id: 'max-width', | |||
| }} | |||
| > | |||
| <MenuItem value={false}>false</MenuItem> | |||
| <MenuItem value="false">false</MenuItem> | |||
There was a problem hiding this comment.
Why changing the type?
There was a problem hiding this comment.
This had to be changed because MenuItem only accepts string | number | string[] | undefined for its type. The change in the handleMaxWidthChange covers converting this back to a boolean when this value is chosen!
There was a problem hiding this comment.
Could MenuItem accept boolean too?
There was a problem hiding this comment.
😩 We're breaking quite a bit of DOM/type safety for some tiny conveniences. We could just use an empty string and check for that. Otherwise we need to override the actual DOM types.
There was a problem hiding this comment.
It's passed straight down to the DOM node, yes. It's probably better to override this with unknown since it is also used as a child of Select. But it's also used without the Select context. In different contexts you might be missing the type checking. I think we should use either some type casting or add the runtime overhead that ensures a sound value type. The value of MenuItem should not be changed.
There was a problem hiding this comment.
So should we keep the value of MenuItem as false and cast it to unknown?
Also, will there be issues trying to read event.target.value?
There was a problem hiding this comment.
event.target.value
They always end up as strings in the the DOM. With the exception of Radio and RadioGroup no component reads this value though. We try to only use the value from props.
So should we keep the value of MenuItem as false and cast it to unknown?
I think the Select already passes the value as the second argument to onChange. That value is the value from the prop and not the DOM. You could use that instead and just force it into state via any/unknown.
There was a problem hiding this comment.
ok I see. So the idea is to mostly use the value returned as the second param in the onChange event? I will push a change!
There was a problem hiding this comment.
Well it depends. If you want the value you passed as a prop you use the second param (type unknown). If you want the value in the DOM you use event.target.value (type string).
eps1lon
added
on hold
|
@sperry94 Sorry for causing more confusion than clarification. The underlying This should help typing |
eps1lon
force-pushed
the
maxWidthDialog-ts-docs
branch
from
eb106d2 to
ffc6f5e
Compare
eps1lon
added
the
component: select
eps1lon
changed the title
|
@eps1lon No worries! I looked at the SelectInput code and now I see what’s going on! The target for the raised event is getting set to contain the value/name from the child. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This PR adds the TS demo for MaxWidthDialog. It was separated from the other PR (#15271) since more changes were involved.