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
261 stars 37 forks source link

Generic Type of Metadata Is Incorrect in nodeRenderer Callback. #192

Open mreid21 opened 1 month ago

mreid21 commented 1 month ago

Describe the bug The type of metadata in nodeRenderer is being inferred incorrectly.

Code That Causes The Issue

<TreeView
        data={data}
        className="basic"
        aria-label="basic example tree"
        nodeRenderer={({ element, getNodeProps, level, handleSelect }) => (
            <div {...getNodeProps()} style={{ paddingLeft: 20 * (level - 1) }}>
                {element.name} {element.metadata?.pending}
            </div>
        )}
    />

To Reproduce I created a minimal reproduction but you can do the following.

  1. Create a tree as outlined in the docs.
  2. add the metadata field to any node in your data.
  3. attempt to access metadata from element in the node renderer prop on the tree.

https://codesandbox.io/p/devbox/8rc7df?file=%2Fsrc%2FBasicTreeView.tsx%3A40%2C5-49%2C7

Expected behavior The generic type of metadata should be inferred in the same manner as it is in flattenTree as shown in my reproduction.

Note: I am almost certain it is caused by this.

interface INode<M extends IFlatMetadata = IFlatMetadata> {
    /** A non-negative integer that uniquely identifies the node */
    id: string | number;
    /** Used to match on key press */
    name: string;
    /** An array with the ids of the children nodes */
    children: (string | number)[];
    /** The parent of the node; null for the root node */
    parent: (string | number) | null;
    /** Used to indicated whether a node is branch to be able load async data onExpand*/
    isBranch?: boolean;
    /** User-defined metadata */
    metadata?: M;
  }

 export interface INodeRendererProps {
    /** The object that represents the rendered node */
    element: INode;
    /** A function which gives back the props to pass to the node */
    getNodeProps: (args?: {
      onClick?: EventCallback;
    }) => IBranchProps | LeafProps;
    /** Whether the rendered node is a branch node */
    isBranch: boolean;
    /** Whether the rendered node is selected */
    isSelected: boolean;
    /** If the node is a branch node, whether it is half-selected, else undefined */
    isHalfSelected: boolean;
    /** If the node is a branch node, whether it is expanded, else undefined */
    isExpanded: boolean;
    /** Whether the rendered node is disabled */
    isDisabled: boolean;
    /** A positive integer that corresponds to the aria-level attribute */
    level: number;
    /** A positive integer that corresponds to the aria-setsize attribute */
    setsize: number;
    /** A positive integer that corresponds to the aria-posinset attribute */
    posinset: number;
    /** Function to assign to the onClick event handler of the element(s) that will toggle the selected state */
    handleSelect: EventCallback;
    /** Function to assign to the onClick event handler of the element(s) that will toggle the expanded state */
    handleExpand: EventCallback;
    /** Function to dispatch actions */
    dispatch: React.Dispatch<TreeViewAction>;
    /** state of the treeview */
    treeState: ITreeViewState;
  }

There is no generic being passed to INodeRenderer so it uses the default specified in INode. You can even see that metadata uses the default type in the repro. It would also explain why there is no type error.

I also reproduced the type issue here: https://www.typescriptlang.org/play/?ssl=37&ssc=1&pln=1&pc=1#code/C4TwDgpgBAkgYgGwIbALIWEgJipUC8UAShAMYD2ATlgDwBQUUAzsJQJYB2A5gDQPOtOXKAB8oHAK4BbAEYRKoqDPLkEEJB0USOWCADNOELIskIEdAHwBuOnU7B5epKWgwAcuV01UUCAA8HHSZYRBR0TBxMAhDkNAxsXAsoAG9+RgB6ACpMqABBcXIOAFoOCC4UNgA3aHsy+ShgAAsUKG02AEcJCAQQKDZdDmA2AwhgpugOT2hM9LS+rAAuAXZuE2k5ShtGDOyoAFUmIwbyKCkUUkaoQqgAawhesEpR4Jm5jiQpCCWWFa4t7ayOVymiQlEoSF6AHc2E0Go0alhguQ9HDoBc2AgsE9NJNdC9ZtsoOjMdilgAKH5CNayeQASgA2gBdf47HIAFXhUDAoIggyuKPGBV0VnEEjMUD0VFRUEoKmAQumBO23OxwHJlNWYkkNMotLWZhZUEB+0OxmAJ04WDYpBQR0h8PGCjwuJqwRk4I4F2OSmgSBkaigCHI2CgSCYIE9UEieEKAFE-NydK9CWwmAAhD0XAD8S2UqnUHENxoO8iKugMpWMnwiuCgye21YSmBzUFQ-wAvrZGLVKE4XLAPLoSDp5PIAAqysDBVKE40c6DkGQAKzI8qaLSej1GvOAY052N0T2MLrrSsY3Qgn0GS3cUw7tjoFA4LAaozVA6mw8P48nwUIyQvK931Sd5PiWAByBwWHAnhG2jJZklFKQlgARgABigds+H6JYACIUNwvhiSxXkliZPgVR3JZTHMdtOzodJ0ncAA1XIABkYAAESgNNYwACVyZiYAAeSIOhQEgKA2RQ6IJIgZFXxYelwMAndwMZZS4NwdTfACXlERSbVqPWeR2ygLMGkoLooGoiBqkoB9GCAA

dgreene1 commented 1 month ago

If you can solve it and make a PR, we’d love the help.

mreid21 commented 1 month ago

I think I can do it.

mreid21 commented 1 month ago

I came up with a fix, but it breaks propTypes. Since the TreeView uses forwardRef, I have to use a type assertion, write a custom forwardRef function, or pass a ref in the component props. The first two approaches break the TreeView.propTypes and the third would be a breaking change.

dgreene1 commented 1 month ago

I think the approach we’d most likely be willing to accept is a custom type assertion since it’s my belief that a custom type assertion would provide the same runtime capabilities of prop types. And it’s better than a breaking change.

Can you submit a PR and tag @mellis481 for review?

btw, if it’s possible to avoid bringing in a library like Zod for type validation, that’s significantly preferable.

mreid21 commented 1 month ago

The main problem is that when I use a type assertion to capture the generic type I get Property 'propTypes' does not exist on type '<T extends IFlatMetadata>(props: ITreeViewProps<T> & { ref?: ForwardedRef<HTMLUListElement> | undefined; }) => Element'

I don't know much about propTypes, maybe I'm missing something in the type assertion?

const TreeView = React.forwardRef(TreeViewInner) as <T extends IFlatMetadata>(
  props: ITreeViewProps<T> & { ref?: React.ForwardedRef<HTMLUListElement> },
) => ReturnType<typeof TreeViewInner>;
mreid21 commented 1 month ago

If I cast forwardRef and do TreeViewInner.propTypes instead of TreeView.propTypes then I still get type inference without the error, but I'm not sure if the propTypes are still captured since I'm doing it on the non forwardRefed version.

dgreene1 commented 1 month ago

I’m sorry that I can’t help. I did the best I was capable of when I wrote the original PR the code that you’re looking at.

mreid21 commented 1 month ago

I created a PR. I'm not sure how propTypes will be affected, but it implements the desired behavior. Once React allows components to accept ref directly, forwardRef can be ditched entirely and this code can become way simpler.