[core] Isolate selectors from different instances

[m4theushw]

Problem

By default, each Reselect selector has an internal cache whose maximum size is 1. When a given selector is called with a given set of arguments it checks the cache to see if there's already a memoized value for that arguments. If yes, returns it. However, if nothing is found, it runs the combiner function (the last argument) and stores the value in the cache. Note that it loses the other memoized values because of the maximum size. This works correctly if there's only one instance of the component in the page. If multiple instances are in the same page, however, this can be a problem because the selectors are shared between all instances and the same goes for their caches. The worst scenario is an infinite loop if the returned value of a selector is listed as the dependency of an effect and this effect also triggers a rerender (e.g. mutating the state). Here's an example using very simple selectors. Looking into the console what is happening is the following:

• render 0 the 1st instance is rendered and sets todos=A (selector cache = empty)
• render 1 the 2nd instance is rendered and sets todos=B (selector cache = A)
• effect 0 the 1st effect runs and schedules a state mutation
• effect 1 the 2nd effect runs and schedules a state mutation
• render 0 the 1st instance rerenders because of the state mutation (selector cache = B)
  • Since the selector cache is different than the last time (B != empty), the selector returned a different value so the effect will also run
• render 1 the 2nd instance rerenders because of the state mutation (selector cache = B)
  • Since the selector cache is different than the last time (B != A), the selector returned a different value so the effect will also run
• effect 0 the 1st effect runs again and schedules a state mutation = infinite loop 💥

Note that "selector cache" means the cache of the selector at the beginning of the render. I used "value" but, with non-primitive values, what is changing is the reference.

To get an example with DataGrid apply this diff and open any docs page with multiple instances

diff --git a/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualScroller.tsx b/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualScroller.tsx
index d15c83ee3..d65957ef6 100644
--- a/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualScroller.tsx
+++ b/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualScroller.tsx
@@ -96,9 +96,8 @@ export const useGridVirtualScroller = (props: UseGridVirtualScrollerProps) => {
    const firstRowIndex = Math.floor(top / rowHeight);
    const lastRowIndex = firstRowIndex + numberOfRowsToRender;

-    const { positions } = gridColumnsMetaSelector(apiRef.current.state); // To avoid infinite loop
-    const firstColumnIndex = getIndexFromScroll(left, positions);
-    const lastColumnIndex = getIndexFromScroll(left + containerWidth!, positions);
+    const firstColumnIndex = getIndexFromScroll(left, columnsMeta.positions);
+    const lastColumnIndex = getIndexFromScroll(left + containerWidth!, columnsMeta.positions);

    return {
      firstRowIndex,
@@ -107,12 +106,12 @@ export const useGridVirtualScroller = (props: UseGridVirtualScrollerProps) => {
      lastColumnIndex,
    };
  }, [
-    apiRef,
-    containerWidth,
-    rootProps.autoHeight,
    disableVirtualization,
-    rowHeight,
+    rootProps.autoHeight,
    currentPage.rows.length,
+    rowHeight,
+    columnsMeta.positions,
+    containerWidth,
    visibleColumns.length,
  ]);

Solution

To address the problem above, my proposal is to isolate the selectors in a way where their caches are not shared between other instances. This means that dummySelector of instance A will not be the same dummySelector of instance B. To achieve that, the idea is to switch the createSelector implementation by a custom own, still based on Reselect, but which memoizes the selectors according to a cache key. The cache key used here is a identifier that is given to each DataGrid instance and it's available as apiRef.current.instanceId. For useGridSelector the change is transparent, since everything was done under the hood. However, when calling a selector directly, e.g. selector(state), now we need to pass apiRef as the first argument instead of the state.

-gridColumnsMetaSelector(apiRef.current.state);
+gridColumnsMetaSelector(apiRef);

Passing state still works but it doesn't use the instance cache, but a shared cache between all instances instead.

The solution here was a bit inspired by https://github.com/toomuchdesign/re-reselect/. But this lib has a different method to get the cache key.

[mui-bot]

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	1,198.4	1,852.6	1,266.3	1,473.38	309.675
Sort 100k rows	ms	704.3	1,064.1	704.3	908.34	138.799
Select 100k rows	ms	141.8	225	205.6	191.82	29.571
Deselect 100k rows	ms	117.5	188.8	176.1	159.18	30.18

Generated by 🚫 dangerJS against 7f0d28c

[flaviendelangle]

I fully agree on the limitation of the current version.
I feel like the perfect solution would not require the user to remember to pass the id when calling the selector directly.

Maybe a solution would be to create a apiRef.current.runSelector and base our demos on it (which could be done later, my idea is just to compare the various solutions)

const pageSize = apiRef.current.runSelector(gridPageSizeSelector)

---

Another approach is to pass the apiRef to the selecto

const pageSize = gridPageSizeSelector(apiRef);

And to avoid breaking changes, still accept apiRef.current.state and differentiate them based on the existence of a given property.
But this does not work well with selectors inside setGridState, so I don't think it's viable.

[m4theushw]

Reselect adds some helper methods to each selector, e.g. resetRecomputations. We can't support them here because I don't have the instance id before the selector is called. This could be understood as a breaking. I don't see it that way because:

1. We don't mention that the selectors are Reselect selectors
2. The helpers are added by Reselect. If they choose to not add them one day we don't need to provide an alternative API
3. Reselect has little to no documentation for them

[m4theushw]

@flaviendelangle Initially I was leaning towards having as the first argument the state and the second one being the cache key. This is how a Reselect or Redux selector works. The main problem is to remember to pass the cache key when not using useGridSelector. I would've created a ESLint to warn when we forget to pass the key. Although, since you suggested the same idea I had and to reduce the number of changed files, I went with the second option: pass apiRef to the selector.

But this does not work well with selectors inside setGridState, so I don't think it's viable.

We can support the following signature and still add the ESLint rule to warn if the instance id is not passed when the selector is used inside setState.

(state: GridState, cacheKey: number | string = 'default') => T

Anyway, the main problem here was with the value of useGridSelector being used as an effect dependency. Selectors called inside methods don't cause the infinite loop so I plan to fix them in a follow-up PR.

[flaviendelangle]

We could use unstable_useId from @mui/utils`

[m4theushw]

I tried. unstable_useId generates the id asynchronously. I need it as soon as possible so even the first selector called is called in its instance cache.

[github-actions]

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