Infer Combobox
type based on onChange
handler
#3798
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where the
Combobox
only inferred the type of the internal value based on thevalue
prop.We only wanted to infer the type of the Combobox based on the
value
prop to make it less confusing where values came from. But if you are using an uncontrolled component then thevalue
is not provided to theCombobox
.In most cases this isn't an actual issue, but it is if you also want to us the
by
prop to improve comparing values based on a certain property.Without this change, the only way of typing the
by
prop correctly is by using an explicit type on theCombobox
:With this change, the internal value type will be inferred by the
onChange
meaning that theby
prop is properly typed now.Test plan
Given a reproduction where the
by
prop is correct if you look at the type of theonChange
:Before:
After:
There is no output because there are no errors.
Given a reproduction where the
by
prop is incorrect if you look at the type of theonChange
:Before:
After:
The error type is much simpler and easier to reason about. It's not as clean as a simple
'id' | 'name'
because theby
prop can also be a function. But at least there isn't a wall of TypeScript errors you have to reason about. Maybe we can improve this part in a future PR.Fixes: #3636