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

[DataGrid] New internal rows structure for v6 #4927

Merged
merged 741 commits into from
Sep 26, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented May 18, 2022

Closes #4851

Nomenclature

The following names can be discussed, I just think it would be nice to have a common term to describe the concepts.

Data Row: Row provided by the user in props.rows / apiRef.current.updateRows / apiRef.current.setRows

Auto Generated Row: Row created by the grid (can be a footer row for aggregation, a group row for row grouping or tree data, a skeleton row for lazy loading, etc...). Those rows don't have a model.

Row: Element on which we apply sorting / filtering / pagination / etc... and that is rendered by the GridRow component (can be either a data row or an auto generated row)

Behavior change

  • We no longer keep the old group expansion when updating the rows through setRows or props.rows.
    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.

  • TreeDataLazyLoading demo is broken until we implement the lazy loading properly (it was a hack which no longer works). The fix should be pretty easy with a new prop similar to AG Grid's isServerSideGroup

In this PR

Row tree format modifications

  • Add a new root group containing all the top level rows and the top level footer
  • Separate the various types of nodes into standalone interfaces (GridLeafNode, GridGroupNode, GridFooterNode with there own properties, we could have a GridSkeletonNode in the future)
  • Store the children of a group grouped by there grouping key inside groupNode.childrenFromPath to be able to see if a path already have a matching child (equivalent to this current groupingCriteriaToIdTree inside buildRowTree)

Row tree management modifications

  • Rename buildRowTree into createRowTree

  • New method updateRowTree

  • New methods insertDataRowInTree and removeDataRowFromTree
    These methods are responsible of inserting / removing data rows in the tree.
    They take care of creating the missing parents / destroying the empty groups.
    They use insertNodeInTree and removeNodeFromTree internally.

  • New methods insertNodeInTree and removeNodeFromTree
    These methods are responsible of inserting / removing node in the tree.
    They take care of registering the node inside its parent's children.

Other state structure modifications

  • Only register the data rows in the lookups / id lists of state.rows.

    • Rename state.rows.ids => state.rows.dataRowIds
    • Rename state.rows.idToIdLookup => state.rows.dataRowIdToIdLookup
    • Rename state.rows.idRowsLookup => state.rows.dataRowIdToModelLookup
  • Remove state.rows.groupingResponseBeforeRowHydration
    Hydration of rows now work like hydration of columns and must be able to remove outdated rows.

  • Replace state.rows.treeDepth with state.rows.treeDepths
    This object now contains the amount of nodes of each rows (that way we can now the depth of the tree after a partial update without re-walking the whole tree)

Performance on partial updates using apiRef.current.updateRows

  • Do not recreate the whole tree for flat tree
  • Do not recreate the whole tree for row grouping
  • Do not recreate the whole tree for tree data

In future PR(s)

  • Performance on partial updates using apiRef.current.updateRows
    • Only filter groups impacted (TODO: which groups are they ?)
    • Only sort groups impacted (TODO: which groups are they ?)
    • Only aggregate groups impacted (TODO: which groups are they ?)

Changelog

Breaking changes

Some selectors related to the rows have been renamed to better described the type of rows they are returning:

- const result = gridRowsIdToIdLookupSelector(apiRef);
+ const result = gridRowsDataRowIdToIdLookupSelector(apiRef);

-const result = gridRowTreeDepthSelector(apiRef);
+ const result = gridRowMaximumTreeDepthSelector(apiRef);

The format of the tree nodes (the element accessible in params.node or with the method apiRef.current.getRowNode) have changed.
You have a new type property, which can be useful for example to apply custom behavior on groups.
Here is an example of the old and new approach to apply a custom value formatter on groups for the grouping column:

<DataGridPremium 
  groupingColDef={() => ({
    valueFormatter: (params) => {
     if (params.id == null) {
       return params.value;
     }

      const rowNode = apiRef.current.getRowNode(params.id!)!;
-     if (rowNode.children?.length) {
+     if (rowNode.type === 'group') {
        return `by ${rowNode.groupingKey ?? ''}`;
      }

      return params.value;
/>

(we should rename gridRowsLookupSelector => gridRowsDataRowIdToModelLookupSelector in another PR to be coherent)

@flaviendelangle flaviendelangle added core Infrastructure work going on behind the scenes breaking change component: data grid This is the name of the generic UI component, not the React module! labels May 18, 2022
@flaviendelangle flaviendelangle self-assigned this May 18, 2022
@mui-bot
Copy link

mui-bot commented May 18, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 498.3 934.9 684.5 698.62 175.881
Sort 100k rows ms 561.4 1,250.1 692.5 875.56 237.344
Select 100k rows ms 189.7 317.8 225.2 242.16 44.959
Deselect 100k rows ms 171.2 306.8 278.2 250.74 50.347

Generated by 🚫 dangerJS against 388ca6a

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 19, 2022
@github-actions

This comment was marked as outdated.

@flaviendelangle flaviendelangle changed the base branch from master to next September 9, 2022 09:35
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 12, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 12, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 12, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 12, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 12, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 13, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 15, 2022
@flaviendelangle
Copy link
Member Author

I'll merge it next week if no review is added :/

@m4theushw
Copy link
Member

It's too complex to review everything. I'm OK with merging it. We'll better see the impacts once we start developing features on top of this new structure. Could you update the description with the breaking changes for the next release?

@shoxter
Copy link

shoxter commented Sep 3, 2024

We no longer keep the old group expansion when updating the rows through setRows or props.rows.
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.

I'm disappointed to see that, despite the fact there was no technical reason to break the existing way the updating/setting of rows previously worked, the decision was made to break this. We use DataGridPremium throughout our business ERP software (around 205 data grids or so) and this is a massive PIA for us for seemingly no gain other than personal preference.

It's decisions like this that make libraries not friendly for larger projects and it seems like, with every update, this is happening with MUI. Breaking something this substantive "just because" is very poor management in my opinion.

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! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Internal row structure for partial filtering / sorting and lazy-loading
5 participants