chore: update types for react #6191
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@bryceosterhaus @ethib137 This LGTM. Let me know if I shouldn't merge it. |
There was a problem hiding this comment.
Hey @bryceosterhaus there are a lot of changes that appear to be more than just changing types. Maybe we should have a call to go over briefly what these changes are and why they are necessary?
| > { | ||
| 'aria-label'?: string; | ||
|
|
||
| 'aria-labelledby'?: string; |
There was a problem hiding this comment.
The old type looked like it was requiring either aria-labelledby or aria-label to be included while also not allowing both to be added at the same time. This doesn't look like this will have the same behavior.
There was a problem hiding this comment.
For some reason with the updated type libraries, it no longer allowed them to be mutually exclusive. I'm not exactly why or what type changed in the react definitions, but I figured it might just be simpler to make them both optional types instead.
| const hasItems = (children?: React.ReactNode) => { | ||
| if (!children) { | ||
| const hasItems = (children?: React.ReactNode | Function) => { | ||
| if (!children || typeof children === 'function') { |
There was a problem hiding this comment.
Because React.Children.map no longer accepts the function type, so we have to guard against that.
Additionally we have to add Function as a suspected type to hasItems because in ClayAutocomplete the children prop can be a function
Essentially there were gaps in the previous types, so I think it just got more strict.
3 participants
This is needed for the dxp migration since dxp uses v18 react types. Lets see what CI says...
Also, we don't necessarily need to merge this here. I can just copy/paste these changes to the dxp migration PR