Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[data grid] Tree data expansion state is lost when data is updated in 6.0.0-beta.1 #7771

Closed
JimMalmborg opened this issue Jan 31, 2023 · 22 comments · Fixed by #8108
Closed
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation feature: Row grouping Related to the data grid Row grouping feature feature: Tree data Related to the data grid Tree data feature plan: Pro Impact at least one Pro user

Comments

@JimMalmborg
Copy link

JimMalmborg commented Jan 31, 2023

It is working as expected in v5 (and 6.0.0-alpha.0).

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/gracious-dream-d69pbp?file=/package.json

  1. Expand "Thomas (6)" row
  2. Click the "Update data" button to update Thomas's job title.

Current behavior 😯

The row data is updated but the expansion state is lost (Thomas is closed).

Expected behavior 🤔

The expansion state should be kept when the data is updated. (Thomas should still be expanded and his job title should be updated)

@flaviendelangle
Copy link
Member

This change was intended (see the first Behavior change of #4927).

If the row update is partial, we recommend to use updateRows instead of updating the prop.
That's something that we can change if needed, but I would be interested to have more information about your use case.

@flaviendelangle flaviendelangle added component: data grid This is the name of the generic UI component, not the React module! status: waiting for author Issue with insufficient information plan: Pro Impact at least one Pro user feature: Tree data Related to the data grid Tree data feature feature: Row grouping Related to the data grid Row grouping feature labels Jan 31, 2023
@JimMalmborg
Copy link
Author

Thanks for the quick answer👍Did not spot this when reading the release notes or upgrade guide. But, updateRows will definitely work for our use case (Live updated tree data).

@github-actions github-actions bot removed the status: waiting for author Issue with insufficient information label Feb 1, 2023
@flaviendelangle
Copy link
Member

@cherniavskii maybe we could add this behavior change to the migration guide

@cherniavskii
Copy link
Member

@flaviendelangle Indeed, we should include this in the migration guide.

We now clearly distinguish full dataset update (setRows and props.rows) and partial dataset update (updateRows).
That part is open for discussion, I like to clearly distinguish the two, but there is no technical limitation to keep the old expansion.

Could you share more details on the motivation behind this behavior change?

@flaviendelangle
Copy link
Member

The first reason was that it was not performance-free to check if the old node in the tree was expended or not.

And the second one is that now we have two very distinct way of changing the rows:

  • update props.rows or call setRows => full regeneration of the tree
  • call updateRows => partial regeneration of the tree

For me it make sense to push people into using updateRows whenever possible because it is by far the cleaner solution when they have a partial update.
I fear that trying to keep some expansion information on full regeneration blurs the line between the two approaches and let people think that using the props.rows update or the setRows method is a good idea.

Basically, the way I see it, props.rows and setRows should only be used for data set change.
In scenario where, you most likely don't want to keep the expansion (because your dataset might have ID collision for totally distinct rows).
And for any other use case, updateRows is the way to go.

@cherniavskii
Copy link
Member

Thanks for the details!
The trade-off sounds good to me, I'll update the migration guide to include this behavior change.

@cihanyakar
Copy link
Contributor

If you still encounter issues after using the updateRows function, the problem might be occurring when you define the getTreeDataPath function as an inline function.

@Enzo-PVsyst
Copy link

Hello, I'm working with DataGridPremium, with collapsable rows (I have a list of items, and I group them by date).

When I expand a row, then create a new item in this date, when the state of options is updated, it closes every rows, how would you make them stay open ?

@mercurydiogo
Copy link

Basically, the way I see it, props.rows and setRows should only be used for data set change. In scenario where, you most likely don't want to keep the expansion (because your dataset might have ID collision for totally distinct rows). And for any other use case, updateRows is the way to go.

I understand and agree with the reasoning. However, when using real-time database updates as row data, having the ability to disable this behavior would be useful. Is there a way to turn it off optionally? Considering the size of the app, transitioning to this could take a considerable amount of time.

@cherniavskii
Copy link
Member

@mercurydiogo

Is there a way to turn it off optionally? Considering the size of the app, transitioning to this could take a considerable amount of time.

Transitioning to using the updateRows method should be fairly easy - see this working demo: https://codesandbox.io/s/peaceful-forest-92w7qt

@AndreaPrillo

This comment was marked as off-topic.

@cherniavskii

This comment was marked as off-topic.

@AndreaPrillo

This comment was marked as off-topic.

@cclews1
Copy link

cclews1 commented Sep 15, 2023

This change was intended (see the first Behavior change of #4927).

If the row update is partial, we recommend to use updateRows instead of updating the prop. That's something that we can change if needed, but I would be interested to have more information about your use case.

@flaviendelangle Is this behavior currently being reflected in the selectRows functionality as well? I'm currently running into an issue where the selectRows function is causing all of the table's expanded groups to collapse. At first I thought it was just on the grouped columns (code here) but the problem persists for ungrouped rows as well.

For context, we are using the onRowSelectionModelChange and rowSelectionModel to control the selected rows.

Disregard - this problem was caused by a lack of memoization of the DataGrid when the parent state update was causing re-render.

@secretwpn
Copy link

Wait a second, so if I have a hassle-free and reasonably cheap way to update whole rows array from server (e.g. on timer) and feed it into DataGrid rows prop - there is no other way to make it keep the expanded/collapsed state between the data updates than the recommended one, which is to handle diffs between old and new data elsewhere via state, refs and such and then notify DataGrid about that via apiRef? And this is by design? Sorry for being bitter but that does not sound user friendly at all.

@cherniavskii
Copy link
Member

@secretwpn No need to handle diffs and state - a single useEffect + useRef is enough - see #7771 (comment)

const initialRowsRef = React.useRef(rows);
React.useEffect(() => {
  if (rows !== initialRowsRef.current) {
    apiRef.current.updateRows(rows);
  }
}, [apiRef, rows]);

@secretwpn
Copy link

secretwpn commented Dec 15, 2023

@cherniavskii thanks, Andrew, I've tried this but it did not work for me - the expanded state kept resetting

I had to go with

interface ExpansionState {
  [key: GridRowId]: boolean;
}
useEffect(() => {
    apiRef.current.subscribeEvent('rowExpansionChange', (node) => {
      expansionState.current[node.id] = node.childrenExpanded ?? false
    })
  }, [apiRef])

  const isGroupExpanded = useCallback(
    (node: GridGroupNode) => expansionState.current[node.id] ?? false,
    [expansionState],
  )
<DataGridPremium
          apiRef={apiRef}
          isGroupExpandedByDefault={isGroupExpanded}
...
/>

as suggested by another comment in one of related discussions #10085

@cherniavskii
Copy link
Member

I've tried this but it did not work for me - the expanded state kept resetting

This is odd... There's also this detail that you may have missed - along with using updateRows in the useEffect, you have to make sure to not update the Data Grid rows prop with the new rows:

<DataGrid
-  rows={rows}
+  rows={initialRowsRef.current}
/>

This could be the reason why the expanded state kept resetting for you.
Hope this helps, let me know if this is not enough for you use case.

@milotoor
Copy link

@cherniavskii or @flaviendelangle are there any plans to make updateRows's retention of expansion-state the default behavior in v7 (or beyond)? It feels cumbersome to me to have to go through the API ref to achieve behavior which seems like it ought to be the default. Given the opinion expressed above...

For me it make sense to push people into using updateRows whenever possible because it is by far the cleaner solution when they have a partial update.
...
Basically, the way I see it, props.rows and setRows should only be used for data set change.
In scenario where, you most likely don't want to keep the expansion (because your dataset might have ID collision for totally distinct rows).
And for any other use case, updateRows is the way to go.

...I'm surprised that updating props.rows is more similar to setRows than it is to updateRows, but I'm probably biased by my own use cases. In my case, the DataGrid is used to show data loaded from a polled API; whenever the polling occurred, the expansion state was being reset. Adopting updateRows seems to resolve the issue.

Right now it seems that the component is treating a change of props.rows to mean a "full dataset update" has occurred. In practice I feel like a "partial dataset update" is more common, but I don't think I truly understand the difference between the two.

Any thoughts or insights are appreciated, and thank you for your work!

@zijunspover
Copy link

If you still encounter issues after using the updateRows function, the problem might be occurring when you define the getTreeDataPath function as an inline function.

It works! By the way, inline function is used in official document. It cost me hours to find this comment, can i get compensation from official? 💢😈

@milotoor
Copy link

milotoor commented Feb 1, 2024

To recap this discussion, here is my understanding of the minimal implementation required to support expansion state retention across dataset updates:

import type { DataGridPremiumProps, GridValidRowModel } from '@mui/x-data-grid-premium'
import { DataGridPremium, useGridApiRef } from '@mui/x-data-grid-premium'
import { useEffect, useRef } from 'react'

function MyTable(props: DataGridPremiumProps<GridValidRowModel>) {
  const { columns, getTreeDataPath, sortingMode, rows, ...dataGridProps } = props
  const apiRef = useGridApiRef()

  // The table data is stored in a ref so that the exact same reference is passed to the
  // `DataGridPremium` component on every render. This ref is never updated. Instead, the
  // rows are updated via the `updateRows` method of the `GridApiPremium` instance. This is
  // done primarily to avoid auto-collapsing any expanded rows when the table data changes.
  const initialData = useRef(rows)

  // Wrap `getRowId` in a ref so that it doesn't trigger the `useEffect` below on every render
  const getRowIdMemo = useRef(props.getRowId!)

  // Handles data updates for the table. When the data changes, the `updateRows` method is called to
  // perform an "upsert" of the new data. This is done to avoid auto-collapsing any expanded rows when
  // the table data changes. However, by default `updateRows` will not remove any rows that are no
  // longer present in the new data. To support removal, we need to merge the new data with the old data
  // and add a `_action: 'delete'` property to any rows that are no longer present.
  useEffect(() => {
    // If we're using server mode, we can't use `updateRows` because the sorting comes out incorrect.
    // This happens because the DataGrid expects sorting to be handled externally (on the server), so
    // after the upsert occurs it does not re-apply the sort model. Instead we just set the rows
    // directly. This means that expansion state will not be preserved for server-side tables.
    if (sortingMode === 'server') {
      apiRef.current.setRows([...rows])
      return
    }

    // Get the IDs of all current data and identify which are excluded from the new data
    const curRows = apiRef.current.getRowModels()
    const curIds = curRows.keys()
    const newIds = new Set(rows.map(getRowIdMemo.current))
    const removeIds = [...curIds].filter((id) => !newIds.has(id))

    // Create delete actions for the removed IDs and merge them with the new data
    const deleteActions = removeIds.map((id) => ({ ...curRows.get(id), _action: 'delete' as const }))
    apiRef.current.updateRows([...rows, ...deleteActions])
  }, [apiRef, rows, sortingMode])

  // To avoid auto-collapsing of expanded rows, the `getTreeDataPath` prop needs to be the same
  // across renders. We wrap it in a ref so that the value provided during the first render is
  // used for all subsequent ones.
  const getTreeDataPathMemo = useRef(getTreeDataPath)

  return (
    <DataGridPremium
      {...dataGridProps}
      apiRef={apiRef}
      columns={columns}
      getTreeDataPath={getTreeDataPathMemo.current}
      rows={initialData.current}
      treeData={!!getTreeDataPath}
    />
  )
}

Without the comments it's about 40 lines of code to implement what I personally feel ought to be the default behavior. There are various gotchas involved:

  • The rows prop needs to be unchanging (this is already highly bizarre to me and I assume to most library users)
  • The getTreeDataPath must be memoized (which, as of 1/31/2024, the documentation's code sample does not do)
  • You have to manually manage data updates using the updateRows function. This function only performs upserts by default, so in order to remove rows from the DataGrid you must merge your new data with the old data, and add the _action: 'delete' field to all obsolete rows
    • Making this easier seems like a very low-hanging fruit. If upserting needs to remain the default behavior, then perhaps updateRows could accept a second parameter to allow for turning on automatic row deletions (see my implementation above, which accomplishes this using only the GridApi object and the getRowId prop--I don't doubt for a moment that there's a cleaner approach)
  • This does not seem to work at all for tables which use a sortingMode of server, because updateRows seems reluctant to change the ordering of rows. Correct me if I'm wrong but this seems to rule out expansion state retention for tables using paginated backends with filtering or sorting.

I'm a big fan of this library and MUI in general, but candidly this API is not very user-friendly. I am happy to provide more information about my use case (though I don't think it's particularly exceptional), and if there are simplifications/corrections to the above please share.

@stivluc
Copy link

stivluc commented Mar 7, 2024

@cherniavskii thanks, Andrew, I've tried this but it did not work for me - the expanded state kept resetting

I had to go with

interface ExpansionState {
  [key: GridRowId]: boolean;
}
useEffect(() => {
    apiRef.current.subscribeEvent('rowExpansionChange', (node) => {
      expansionState.current[node.id] = node.childrenExpanded ?? false
    })
  }, [apiRef])

  const isGroupExpanded = useCallback(
    (node: GridGroupNode) => expansionState.current[node.id] ?? false,
    [expansionState],
  )
<DataGridPremium
          apiRef={apiRef}
          isGroupExpandedByDefault={isGroupExpanded}
...
/>

as suggested by another comment in one of related discussions #10085

Hi, thanks for your solution, it's working well. However, it seems not to work with defaultGroupingExpansionDepth. Do you know how to combine both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation feature: Row grouping Related to the data grid Row grouping feature feature: Tree data Related to the data grid Tree data feature plan: Pro Impact at least one Pro user
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.