Add additional type-casts for React nodes #7402
Conversation
|
Preview: https://patternfly-react-pr-7402.surge.sh A11y report: https://patternfly-react-pr-7402-a11y.surge.sh |
|
@nicolethoen @tlabaj could this get reviewed and merged? I'd like to rebase the React 18 PR on top. |
| @@ -271,16 +275,20 @@ export class DualListSelector extends React.Component<DualListSelectorProps, Dua | |||
| this.setState(prevState => { | |||
| const movedOptions = | |||
| prevState.availableTreeFilteredOptions || | |||
| flattenTreeWithFolders(prevState.availableOptions as DualListSelectorTreeItemData[]); | |||
| flattenTreeWithFolders((prevState.availableOptions as unknown) as DualListSelectorTreeItemData[]); | |||
There was a problem hiding this comment.
Curious. Why unknown here and not any?
There was a problem hiding this comment.
The unknown keyword still allows a stricter subset of type checking, whereas any disables all checks. But in this case I used unknown as it was the recommendation by the compiler.
There was a problem hiding this comment.
So im realizing that you're needing to cast these list as unknown since the chosenOptions and availableOptions props are both typed as React.ReactNode[] | DualListSelectorTreeItemData[].
It seems as though, since the tree feature was added later, we could tighten the typing of these various lists of options used throughout the component to avoid using the 'unknown' casting. Unless casting to unknown was something recommended as part of the migration to react 18 and is a fine solution. WDYT?
|
@nicolethoen dual list selector is still Beta so we can make improvements to API that are "breaking" I do not know if that is blocker for this PR though. we could open a follow up issue to address that. @jonkoops any thought on this? |
|
Sorry for the late response, I have been focused on some other projects so I haven't had the time to get back to this.
Yes, I think it makes total sense to re-work these types. Ideally there would never be any
Agreed, let's open up an issue for this. Especially if this is still a 'beta' feature I think it makes sense to stabilize this before making it set in stone. I think we can merge this as-is so I can continue creating new PRs to help with the React 18 migration. WDYT @nicolethoen @tlabaj? |
|
I opened this follow up issue for the dual list selector |
Since React 18 has stricter types than before some type-casts need to be added and expanded in order for the code to compile successfully.
Needed to land #7142