dgreene1
fix for broken inference on metadata prop
This should provide type inference for element.metadata. I checked to see that the type was correct when passing metadata. However, I don't know too much about propTypes since I only really use typescript with React. Please let me know if there is any issues with this approach.
fixes #192
@mreid21 unless I’m mistaken, I don’t see any code that does a type assertion in that there is no code that throws if the ref is not present.
function genericForwardRef<T, P = Record<string, unknown>>(
render: (props: P, ref: React.Ref<T>) => JSX.Element
): (props: P & React.RefAttributes<T>) => JSX.Element {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return React.forwardRef(render) as any;
}
I'm not sure what you mean, maybe I'm misunderstanding? I do a type assertion here. If I perform the type assertion directly on forwardRef propTypes throws an error because propTypes does not exist on my asserted type. To fix this, I rewrite forwardRef so that the generic from ITreeViewProps is captured. Since this is all done at the type level I know none of the functionality should change.
One downside is that the correct type for metadata cannot be inferred in flatten tree. So the only reliable way is to set the generic on flattenTree. It will be inferred in the component from that point though.
@dgreene1 @mellis481 I updated the PR to use a type assertion. You can see that if you add the metadata property to any of the example tests that the type of metadata will now be correctly inferred by nodeRenderer. Please take a look, as this change will be super helpful for typescript consumers and shouldn't have any impact on runtime performance or js consumers.
Plus one on this PR, I have to do an ugly hack to achieve the same thing:
export interface NodeRendererProps<M extends IFlatMetadata>
extends Omit<INodeRendererProps, 'element'> {
element: INode<M>
}
interface TreeViewProps<M extends IFlatMetadata> extends Omit<ITreeViewProps, 'nodeRenderer'> {
data: INode<M>[]
nodeRenderer: (props: NodeRendererProps<M>) => React.ReactNode
}
function TreeView<M extends IFlatMetadata>(props: TreeViewProps<M>) {
return <TreeViewPrimitive {...(props as any)} />
}
@mreid21 Can you address the failing PR check please? I'll try to have my team review/merge your PR soon. Thanks.
@mreid21, thanks for evaluating my comments and updating the PR.
should I assume that any function that accepts INode should instead be generic INode<M>
No, because INode already has a generic with a default type. As you mentioned, not all functions are used by the consumer directly, so we can skip as such. But I'd avoid type-casting where possible, e.g., getTreeNode and making too many changes at once.
On the other hand, I've found it advisable to use the default type in Node and NodeGroup components since we also used a default type in the TreeView component.
@mellis481, @dgreene1, this PR looks good to me. Let's see if checks pass after changes.
@mreid21 Released in 2.10.0. Thanks for your contribution. Thanks for your patience. In the future, we will work to get PRs reviewed more quickly.
CC: @danielbuechele