mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.57k stars 1.34k forks source link

[TreeView] expandedItems requires reference changing #15161

Open tamayika opened 1 month ago

tamayika commented 1 month ago

Steps to reproduce

Link to live example: (required)

Steps:

  1. Run below component

    function Sample() {
    const [expandedItems, setExpandedItems] = useState<string[]>([]);
    
    return (
    <>
      <Button
        onClick={() =>
          setExpandedItems((prev) => {
            prev.push("item1");
            return prev;
          })
        }
      >
        Expand
      </Button>
      <SimpleTreeView
        expandedItems={expandedItems}
        onExpandedItemsChange={(_, itemIds) => setExpandedItems(itemIds)}
      >
        <TreeItem itemId="item1" label="item1">
          <TreeItem itemId="item2" label="item2" />
        </TreeItem>
      </SimpleTreeView>
    </>
    );
    }
  2. Press Expand Button

Current behavior

Root TreeItem is not expanded.

Expected behavior

Root TreeItem is expanded.

Context

I'm upgrading @mui/lab from 5.0.0-alpha.81 to 6.0.0-beta.13. And TreeView is deprecated so I moved to @mui/x-tree-view. But SimpleTreeView's expandedItems looks its array reference, so example code does not work after upgrade. I'm using mobx to manage states and array reference is immutable in item's addition.

I think this behaviour is intended by component optimization, but documentation is necessary.

Before: https://github.com/mui/material-ui/blob/v5.4.4/packages/mui-lab/src/TreeView/TreeView.js#L125-L128 After: https://github.com/mui/mui-x/blob/master/packages/x-tree-view/src/internals/plugins/useTreeViewExpansion/useTreeViewExpansion.ts#L12-L19

Your environment

npx @mui/envinfo ``` System: OS: Linux 5.15 Ubuntu 22.04.1 LTS 22.04.1 LTS (Jammy Jellyfish) Binaries: Node: 22.2.0 - ~/.anyenv/envs/nodenv/versions/22.2.0/bin/node npm: 10.7.0 - ~/.anyenv/envs/nodenv/versions/22.2.0/bin/npm pnpm: Not Found Browsers: Chrome: 121.0.6167.139 npmPackages: @emotion/react: ^11.13.3 => 11.13.3 @emotion/styled: ^11.13.0 => 11.13.0 @mui/base: 5.0.0-beta.60 @mui/core-downloads-tracker: 6.1.5 @mui/icons-material: ^6.1.5 => 6.1.5 @mui/lab: 6.0.0-beta.13 => 6.0.0-beta.13 @mui/material: ^6.1.5 => 6.1.5 @mui/private-theming: 6.1.5 @mui/styled-engine: 6.1.5 @mui/styles: ^6.1.5 => 6.1.5 @mui/system: 6.1.5 @mui/types: 7.2.18 @mui/utils: 6.1.5 @mui/x-data-grid: ^7.21.0 => 7.21.0 @mui/x-internals: 7.21.0 @mui/x-tree-view: ^7.21.0 => 7.21.0 @types/react: ^18.2.9 => 18.2.9 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^5.6.3 => 5.6.3 ```

Search keywords: TreeView expandedItems

flaviendelangle commented 1 month ago

Hi,

The behavior is indeed intended, we are applying logic when the array changes (using a regular useMemo), so if the array is mutated, the derived value won't be re-computed.

Regarding documentation, I tried adding readonly to the expandedItems prop, but TypeScript does not seem to be smart enough to complain when you pass a mutable state (and even if it did, not sure we should enforce defining the state as readonly string[] in people's code, the vast vast majority of people are used to not mutating value).

We could discuss adding a small warning on the documentation of each model (expansion, selection and items for RichTreeView). I'm adding it to our board :+1: