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

[core] Remove rowsCache from state #4480

Merged
merged 3 commits into from
May 5, 2022

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Apr 13, 2022

Fixes the following TODO by removing the cache from the state and moving it to apiRef. In #3896 I added it to the state because was the simplest thing to do.

rowsCache, // TODO remove from state

I also converted the variables used to track the timeout into refs.

Instead of using apiRef, another alternative could be the following:

diff --git a/packages/grid/x-data-grid/src/DataGrid/useDataGridComponent.tsx b/packages/grid/x-data-grid/src/DataGrid/useDataGridComponent.tsx
index b617e664b..88acba309 100644
--- a/packages/grid/x-data-grid/src/DataGrid/useDataGridComponent.tsx
+++ b/packages/grid/x-data-grid/src/DataGrid/useDataGridComponent.tsx
@@ -50,6 +50,7 @@ import { useGridColumnSpanning } from '../hooks/features/columns/useGridColumnSp
 
 export const useDataGridComponent = (props: DataGridProcessedProps) => {
   const apiRef = useGridInitialization<GridApiCommunity>(undefined, props);
+  const rowsCache = React.useRef();
 
   /**
    * Register all pre-processors called during state initialization here.
@@ -62,7 +63,7 @@ export const useDataGridComponent = (props: DataGridProcessedProps) => {
    */
   useGridInitializeState(selectionStateInitializer, apiRef, props);
   useGridInitializeState(columnsStateInitializer, apiRef, props);
-  useGridInitializeState(rowsStateInitializer, apiRef, props);
+  useGridInitializeState(rowsStateInitializer, apiRef, props, { rowsCache });
   useGridInitializeState(
     props.experimentalFeatures?.newEditingApi
       ? editingStateInitializer_new

@mui-bot
Copy link

mui-bot commented Apr 13, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 258.4 505.6 434.8 391.18 109.945
Sort 100k rows ms 496.6 852.2 708.2 687.56 127.817
Select 100k rows ms 128.3 231.2 151.5 165.36 37.954
Deselect 100k rows ms 100.9 279.6 184.3 187.06 57.082

Generated by 🚫 dangerJS against 37dbfcc

@m4theushw m4theushw force-pushed the remove-rows-cache-from-state branch from 8132c6c to 91854f3 Compare April 13, 2022 01:12
@m4theushw m4theushw marked this pull request as ready for review April 13, 2022 03:03
@m4theushw m4theushw added the core Infrastructure work going on behind the scenes label Apr 13, 2022
ids: [],
},
rowsBeforePartialUpdates: [],
apiRef.current.rowsCache = convertRowsPropToState({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep it in the apiRef, what do you think about doing apiRef.current.caches.rows ?
Or apiRef.current.unstable_caches.rows if we want to keep it private for now.

I would like to move unstable_sanitizedModelOnLastRowTreeCreation out of the state (same for the equivalent of the Aggregation)
Maybe it's worth scoping it instead of doing it on the root (same for the DOM refs btw)

For the alternative you gave in the PR description, it don't know how we could adapt it in the future to allow custom plugins with there own cache without touching the useDataGridXXXComponent hook.
But as long as the caches system is private, we can rewrite it to be compatible when we know what the plugins will actually look like

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with unstable_caches. I hope that theme augmentation works to enrich the cache interface to allow custom plugins to have their own cache.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work perfectly

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

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

@flaviendelangle
Copy link
Member

I think we can go on with this one, #4208 will need a very similar implementation (I did a custom one for now to have something that works)

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 3, 2022
@m4theushw m4theushw requested a review from flaviendelangle May 3, 2022 21:17
@m4theushw m4theushw merged commit a5873c2 into mui:master May 5, 2022
@m4theushw m4theushw deleted the remove-rows-cache-from-state branch May 5, 2022 16:59
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants