dgreene1 / react-accessible-treeview

A react component that implements the treeview pattern as described by the WAI-ARIA Authoring Practices.
https://dgreene1.github.io/react-accessible-treeview
MIT License
277 stars 37 forks source link

fix for broken inference on metadata prop #193

Open mreid21 opened 2 months ago

mreid21 commented 2 months ago

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 commented 2 months ago

@mellis481

dgreene1 commented 2 months ago

@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.

mreid21 commented 2 months ago
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.

mreid21 commented 2 months ago

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.

mreid21 commented 2 months ago

@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.

ivasilov commented 4 weeks ago

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)} />
}
mellis481 commented 1 week ago

@mreid21 Can you address the failing PR check please? I'll try to have my team review/merge your PR soon. Thanks.

mreid21 commented 1 week ago

I added the missing generics for Node. All the tests are passing now.