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] Rows, grid headers, and cells re-render on RenderContext change #15618

Open
lauri865 opened this issue Nov 26, 2024 · 3 comments
Open
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine

Comments

@lauri865
Copy link
Contributor

lauri865 commented Nov 26, 2024

Steps to reproduce

Steps:

  1. Open this link to live example:
    2.Turn on react update highlights in the devtools
    3.Scroll enough to trigger RenderContext change (horizontally for GridHeaderCells and vertically for GridRows)

Current behavior

Too many re-renders, making memoization mostly an overhead with little gains.

Expected behavior

The problem stems from too many positional props being passed into these components, but in reality:

  • Row needs to only now first and last column index, not rowindexes
  • Cell needs to only know whether it should draw borders or not (booleans rather than a plethora of positional props that change more often)
  • Headers should have a pinnedOffset prop instead of a style object that changes on each re-render (GridCells have suchan implementation)

This should be calculated on GridRow level and passed as boolean props:
https://github.com/mui/mui-x/blob/721b285db4662de5c33e139c4b4cc4d63ae90019/packages/x-data-grid/src/components/cell/GridCell.tsx#L274C3-L281C5

Same here:
https://github.com/mui/mui-x/blob/721b285db4662de5c33e139c4b4cc4d63ae90019/packages/x-data-grid/src/components/columnHeaders/GridColumnHeaderItem.tsx#L138C3-L145C5

RenderContext should be removed and replaced with firstColumnIndex and lastColumnIndex:

export interface GridRowProps extends React.HTMLAttributes<HTMLDivElement> {
row: GridRowModel;
rowId: GridRowId;
selected: boolean;
/**
* Index of the row in the whole sorted and filtered dataset.
* If some rows above have expanded children, this index also take those children into account.
*/
index: number;
rowHeight: number | 'auto';
offsetTop: number | undefined;
offsetLeft: number;
dimensions: GridDimensions;
renderContext: GridRenderContext;

Style object should be replaced with pinnedOffset, similar to how GridCells handle this:

Should be easy enough fixes, but I don't have time myself to create a PR right now.

Maybe an eslint rule to prevent object props on these hot code paths would be helpful as well, because using non-stable objects like renderContext or style will just break fastMemo and potentially make it worse to memoize than not do it at all.

Context

No response

Your environment

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Search keywords: re-render

@lauri865 lauri865 added bug 🐛 Something doesn't work status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 26, 2024
@github-actions github-actions bot added the component: data grid This is the name of the generic UI component, not the React module! label Nov 26, 2024
@lauri865
Copy link
Contributor Author

@michelengelen michelengelen changed the title [Datagrid] Rows, grid headers, and cells re-render on RenderContext change [data grid] Rows, grid headers, and cells re-render on RenderContext change Nov 26, 2024
@michelengelen
Copy link
Member

Thanks for opening this and the detailed explanation @lauri865 ... I'll add this to the board for the team to pick up! 👍🏼

@michelengelen michelengelen added feature: Rendering layout Related to the data grid Rendering engine and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 26, 2024
@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Nov 26, 2024
@romgrk
Copy link
Contributor

romgrk commented Nov 26, 2024

We should implement a regression test, we keep fixing & breaking re-renders, and it affects performance substantially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine
Projects
Status: 🆕 Needs refinement
Development

No branches or pull requests

3 participants