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] Fix memoized selectors with arguments (#15336) @MBilalShafi #15336

Merged
merged 1 commit into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ module.exports = {
),
},
],
// TODO remove rule from here once it's merged in `@mui/monorepo/.eslintrc`
'@typescript-eslint/no-unused-vars': [
'error',
{ vars: 'all', args: 'after-used', ignoreRestSiblings: true, argsIgnorePattern: '^_' },
],
// TODO move rule into the main repo once it has upgraded
'@typescript-eslint/return-await': 'off',
'no-restricted-imports': 'off',
Expand Down
1 change: 0 additions & 1 deletion packages/x-data-grid/src/components/GridDetailPanels.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ export interface GridDetailPanelsProps {
virtualScroller: VirtualScroller;
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
export function GridDetailPanels(_: GridDetailPanelsProps) {
return null;
}
1 change: 0 additions & 1 deletion packages/x-data-grid/src/components/GridPinnedRows.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ export interface GridPinnedRowsProps {
virtualScroller: VirtualScroller;
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
export function GridPinnedRows(_: GridPinnedRowsProps) {
return null;
}
41 changes: 37 additions & 4 deletions packages/x-data-grid/src/hooks/utils/useGridSelector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import { fastObjectShallowCompare } from '@mui/x-internals/fastObjectShallowCompare';
import { warnOnce } from '@mui/x-internals/warning';
import type { GridApiCommon } from '../../models/api/gridApiCommon';
import { OutputSelector, OutputSelectorV8 } from '../../utils/createSelector';
import type { OutputSelector, OutputSelectorV8 } from '../../utils/createSelector';
import { useLazyRef } from './useLazyRef';
import { useOnMount } from './useOnMount';
import type { GridCoreApi } from '../../models/api/gridCoreApi';
Expand Down Expand Up @@ -43,8 +43,25 @@ function applySelectorV8<Api extends GridApiCommon, Args, T>(

const defaultCompare = Object.is;
export const objectShallowCompare = fastObjectShallowCompare;
const arrayShallowCompare = (a: any[], b: any[]) => {
if (a === b) {
return true;
}

return a.length === b.length && a.every((v, i) => v === b[i]);
};

const createRefs = () => ({ state: null, equals: null, selector: null }) as any;
export const argsEqual = (prev: any, curr: any) => {
let fn = Object.is;
if (curr instanceof Array) {
fn = arrayShallowCompare;
} else if (curr instanceof Object) {
fn = objectShallowCompare;
}
return fn(prev, curr);
};

const createRefs = () => ({ state: null, equals: null, selector: null, args: null }) as any;

// TODO v8: Remove this function
export const useGridSelector = <Api extends GridApiCommon, T>(
Expand Down Expand Up @@ -98,7 +115,7 @@ export const useGridSelectorV8 = <Api extends GridApiCommon, Args, T>(
apiRef: React.MutableRefObject<Api>,
selector: Selector<Api, Args, T>,
args: Args = undefined as Args,
equals: (a: T, b: T) => boolean = defaultCompare,
equals: <U = T>(a: U, b: U) => boolean = defaultCompare,
) => {
if (process.env.NODE_ENV !== 'production') {
if (!apiRef.current.state) {
Expand All @@ -114,6 +131,7 @@ export const useGridSelectorV8 = <Api extends GridApiCommon, Args, T>(
state: T;
equals: typeof equals;
selector: typeof selector;
args: typeof args;
},
never
>(createRefs);
Expand All @@ -127,13 +145,28 @@ export const useGridSelectorV8 = <Api extends GridApiCommon, Args, T>(
refs.current.state = state;
refs.current.equals = equals;
refs.current.selector = selector;
const prevArgs = refs.current.args;
refs.current.args = args;

if (didInit && !argsEqual(prevArgs, args)) {
const newState = applySelectorV8(
apiRef,
refs.current.selector,
refs.current.args,
apiRef.current.instanceId,
) as T;
if (!refs.current.equals(refs.current.state, newState)) {
refs.current.state = newState;
setState(newState);
}
}

useOnMount(() => {
return apiRef.current.store.subscribe(() => {
const newState = applySelectorV8(
apiRef,
refs.current.selector,
args,
refs.current.args,
apiRef.current.instanceId,
) as T;
if (!refs.current.equals(refs.current.state, newState)) {
Expand Down
25 changes: 23 additions & 2 deletions packages/x-data-grid/src/utils/createSelector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as React from 'react';
import { lruMemoize, createSelectorCreator, Selector, SelectorResultArray } from 'reselect';
import { warnOnce } from '@mui/x-internals/warning';
import type { GridCoreApi } from '../models/api/gridCoreApi';
import { argsEqual } from '../hooks/utils/useGridSelector';

type CacheKey = { id: number };

Expand All @@ -13,6 +14,10 @@ const reselectCreateSelector = createSelectorCreator({
},
});

type GridCreateSelectorFunction = ReturnType<typeof reselectCreateSelector> & {
selectorArgs?: any;
};

// TODO v8: Remove this type
export interface OutputSelector<State, Result> {
(apiRef: React.MutableRefObject<{ state: State; instanceId: GridCoreApi['instanceId'] }>): Result;
Expand All @@ -24,7 +29,7 @@ export interface OutputSelector<State, Result> {
export interface OutputSelectorV8<State, Args, Result> {
(
apiRef: React.MutableRefObject<{ state: State; instanceId: GridCoreApi['instanceId'] }>,
args: Args,
args?: Args,
): Result;
(state: State, instanceId: GridCoreApi['instanceId']): Result;
acceptsApiRef: boolean;
Expand Down Expand Up @@ -323,12 +328,28 @@ export const createSelectorMemoizedV8: CreateSelectorFunctionV8 = (...args: any)
const cacheFn = cacheArgs?.get(args);

if (cacheArgs && cacheFn) {
if (!argsEqual(cacheFn.selectorArgs, selectorArgs)) {
const reselectArgs =
selectorArgs !== undefined
? [...args.slice(0, args.length - 1), () => selectorArgs, args[args.length - 1]]
: args;
const fn: GridCreateSelectorFunction = reselectCreateSelector(...reselectArgs);
fn.selectorArgs = selectorArgs;
cacheArgs.set(args, fn);
return fn(state, selectorArgs, cacheKey);
}
// We pass the cache key because the called selector might have as
// dependency another selector created with this `createSelector`.
return cacheFn(state, selectorArgs, cacheKey);
}

const fn = reselectCreateSelector(...args);
const reselectArgs =
selectorArgs !== undefined
? [...args.slice(0, args.length - 1), () => selectorArgs, args[args.length - 1]]
: args;

const fn: GridCreateSelectorFunction = reselectCreateSelector(...reselectArgs);
fn.selectorArgs = selectorArgs;

if (!cacheArgsInit) {
cache.set(cacheKey, cacheArgs);
Expand Down
2 changes: 0 additions & 2 deletions packages/x-data-grid/src/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,9 @@ export function deepClone(obj: Record<string, any>) {
return JSON.parse(JSON.stringify(obj));
}

/* eslint-disable @typescript-eslint/no-unused-vars */
/**
* Mark a value as used so eslint doesn't complain. Use this instead
* of a `eslint-disable-next-line react-hooks/exhaustive-deps` because
* that hint disables checks on all values instead of just one.
*/
export function eslintUseValue(_: any) {}
/* eslint-enable @typescript-eslint/no-unused-vars */