-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Upgrade React Select #16027
Upgrade React Select #16027
Conversation
A few notes:
|
@@ -24,7 +24,8 @@ const Separator = styled.div` | |||
padding: 0 5px; | |||
`; | |||
|
|||
const SingleValue: React.FC<IProps> = (props) => { | |||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
const SingleValue: React.FC<IProps<any>> = (props) => { |
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 the weird component prop-drilling could be refactored out so these could be correctly typed.
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.
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.
While I agree with splitting this apart if possible, is there any way we can go with unknown
for now. The any
here currently actually allows us to access props.data.value
below, while props.data
could actually be undefined (at least according to types) and then throw us an exception at runtime.
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.
Changed!
|
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.
3b8817b
to
e06117c
Compare
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.
Tested locally and did not find any visual or functional regression
* I'm not in love with the end result here, but it's been upgraded and compiles. * Fixing overflow text issue * Tim CR
* I'm not in love with the end result here, but it's been upgraded and compiles. * Fixing overflow text issue * Tim CR
Resolves #16023
The old react-select setup made a lot of assumptions, abused certain facets of the library, and had some weird typing errors.
This upgrade is intended to put us in a better position for changes and improvements in the future. A number of the assumptions that were made still exist and will likely need a full rewrite in the future to increase maintainability.
For the future of our use of react-select, I feel like we should be able to get away from the whole
components={components}
and instead reference things more simply, and with better types. That would be a larger refactor that what's reasonable here however.