From 782d560bf55c50c9af997d1ae14af927c62867eb Mon Sep 17 00:00:00 2001 From: Bilal Shafi Date: Thu, 31 Oct 2024 00:23:45 +0500 Subject: [PATCH 1/8] [DataGrid] Improve v8 selectors --- .../src/hooks/utils/useGridSelector.ts | 13 ++++++++--- .../x-data-grid/src/utils/createSelector.ts | 22 ++++++++++++++++++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts index e9ac0e0f44bd..a69ccef47c14 100644 --- a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts +++ b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts @@ -44,7 +44,7 @@ function applySelectorV8( const defaultCompare = Object.is; export const objectShallowCompare = fastObjectShallowCompare; -const createRefs = () => ({ state: null, equals: null, selector: null }) as any; +const createRefs = () => ({ state: null, equals: null, selector: null, args: null }) as any; // TODO v8: Remove this function export const useGridSelector = ( @@ -98,7 +98,7 @@ export const useGridSelectorV8 = ( apiRef: React.MutableRefObject, selector: Selector, args: Args = undefined as Args, - equals: (a: T, b: T) => boolean = defaultCompare, + equals: (a: U, b: U) => boolean = defaultCompare, ) => { if (process.env.NODE_ENV !== 'production') { if (!apiRef.current.state) { @@ -114,6 +114,7 @@ export const useGridSelectorV8 = ( state: T; equals: typeof equals; selector: typeof selector; + args: typeof args; }, never >(createRefs); @@ -127,13 +128,19 @@ export const useGridSelectorV8 = ( refs.current.state = state; refs.current.equals = equals; refs.current.selector = selector; + const prevArgs = refs.current.args; + refs.current.args = args; + if (didInit && !refs.current.equals(prevArgs, args)) { + // React to arguments change + apiRef.current.store.update(apiRef.current.state); + } 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)) { diff --git a/packages/x-data-grid/src/utils/createSelector.ts b/packages/x-data-grid/src/utils/createSelector.ts index 61f343783780..44527b1a6ab8 100644 --- a/packages/x-data-grid/src/utils/createSelector.ts +++ b/packages/x-data-grid/src/utils/createSelector.ts @@ -13,6 +13,10 @@ const reselectCreateSelector = createSelectorCreator({ }, }); +type GridCreateSelectorFunction = ReturnType & { + selectorArgs?: any; +}; + // TODO v8: Remove this type export interface OutputSelector { (apiRef: React.MutableRefObject<{ state: State; instanceId: GridCoreApi['instanceId'] }>): Result; @@ -323,12 +327,28 @@ export const createSelectorMemoizedV8: CreateSelectorFunctionV8 = (...args: any) const cacheFn = cacheArgs?.get(args); if (cacheArgs && cacheFn) { + if (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); From 5e111b8738f768e94457b9a59f77bf07253a8504 Mon Sep 17 00:00:00 2001 From: Bilal Shafi Date: Thu, 31 Oct 2024 16:52:41 +0500 Subject: [PATCH 2/8] Only recompute current selector on args change --- .../x-data-grid/src/hooks/utils/useGridSelector.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts index a69ccef47c14..f50fee4403d5 100644 --- a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts +++ b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts @@ -131,8 +131,16 @@ export const useGridSelectorV8 = ( const prevArgs = refs.current.args; refs.current.args = args; if (didInit && !refs.current.equals(prevArgs, args)) { - // React to arguments change - apiRef.current.store.update(apiRef.current.state); + 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(() => { From 912e09d949d599e1d4c210b714a1ce2e3f4d3f0d Mon Sep 17 00:00:00 2001 From: Bilal Shafi Date: Fri, 1 Nov 2024 00:15:41 +0500 Subject: [PATCH 3/8] Add shallow comparison for complex arguments --- .../src/hooks/utils/useGridSelector.ts | 36 +++++++++++++------ 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts index f50fee4403d5..225995b98e2d 100644 --- a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts +++ b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts @@ -43,6 +43,13 @@ function applySelectorV8( 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, args: null }) as any; @@ -130,16 +137,25 @@ export const useGridSelectorV8 = ( refs.current.selector = selector; const prevArgs = refs.current.args; refs.current.args = args; - if (didInit && !refs.current.equals(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); + + if (didInit) { + let argsEqual = Object.is; + if (args instanceof Array) { + argsEqual = arrayShallowCompare; + } else if (args instanceof Object) { + argsEqual = objectShallowCompare; + } + if (!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); + } } } From 169469818363c93853933cbe3822f59f352eeba7 Mon Sep 17 00:00:00 2001 From: Bilal Shafi Date: Fri, 1 Nov 2024 18:03:53 +0500 Subject: [PATCH 4/8] Fix types issue + other updates --- .../src/hooks/utils/useGridSelector.ts | 38 ++++++++++--------- .../x-data-grid/src/utils/createSelector.ts | 5 ++- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts index 225995b98e2d..23b575eae01d 100644 --- a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts +++ b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts @@ -51,6 +51,16 @@ const arrayShallowCompare = (a: any[], b: any[]) => { return a.length === b.length && a.every((v, i) => v === b[i]); }; +export const areArgsEqual = (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 @@ -138,24 +148,16 @@ export const useGridSelectorV8 = ( const prevArgs = refs.current.args; refs.current.args = args; - if (didInit) { - let argsEqual = Object.is; - if (args instanceof Array) { - argsEqual = arrayShallowCompare; - } else if (args instanceof Object) { - argsEqual = objectShallowCompare; - } - if (!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); - } + if (didInit && !areArgsEqual(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); } } diff --git a/packages/x-data-grid/src/utils/createSelector.ts b/packages/x-data-grid/src/utils/createSelector.ts index 44527b1a6ab8..281daba26eab 100644 --- a/packages/x-data-grid/src/utils/createSelector.ts +++ b/packages/x-data-grid/src/utils/createSelector.ts @@ -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 { areArgsEqual } from '../hooks/utils/useGridSelector'; type CacheKey = { id: number }; @@ -28,7 +29,7 @@ export interface OutputSelector { export interface OutputSelectorV8 { ( apiRef: React.MutableRefObject<{ state: State; instanceId: GridCoreApi['instanceId'] }>, - args: Args, + args?: Args, ): Result; (state: State, instanceId: GridCoreApi['instanceId']): Result; acceptsApiRef: boolean; @@ -327,7 +328,7 @@ export const createSelectorMemoizedV8: CreateSelectorFunctionV8 = (...args: any) const cacheFn = cacheArgs?.get(args); if (cacheArgs && cacheFn) { - if (cacheFn.selectorArgs !== selectorArgs) { + if (!areArgsEqual(cacheFn.selectorArgs, selectorArgs)) { const reselectArgs = selectorArgs !== undefined ? [...args.slice(0, args.length - 1), () => selectorArgs, args[args.length - 1]] From 2c438567027cb47b994247f3172dec770d4c8e08 Mon Sep 17 00:00:00 2001 From: Bilal Shafi Date: Fri, 1 Nov 2024 18:45:12 +0500 Subject: [PATCH 5/8] Fix eslint --- packages/x-data-grid/src/hooks/utils/useGridSelector.ts | 6 +++--- packages/x-data-grid/src/utils/createSelector.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts index 23b575eae01d..013b904447ee 100644 --- a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts +++ b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts @@ -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'; @@ -51,7 +51,7 @@ const arrayShallowCompare = (a: any[], b: any[]) => { return a.length === b.length && a.every((v, i) => v === b[i]); }; -export const areArgsEqual = (prev: any, curr: any) => { +export const argsEqual = (prev: any, curr: any) => { let fn = Object.is; if (curr instanceof Array) { fn = arrayShallowCompare; @@ -148,7 +148,7 @@ export const useGridSelectorV8 = ( const prevArgs = refs.current.args; refs.current.args = args; - if (didInit && !areArgsEqual(prevArgs, args)) { + if (didInit && !argsEqual(prevArgs, args)) { const newState = applySelectorV8( apiRef, refs.current.selector, diff --git a/packages/x-data-grid/src/utils/createSelector.ts b/packages/x-data-grid/src/utils/createSelector.ts index 281daba26eab..ec6c0bf352de 100644 --- a/packages/x-data-grid/src/utils/createSelector.ts +++ b/packages/x-data-grid/src/utils/createSelector.ts @@ -2,7 +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 { areArgsEqual } from '../hooks/utils/useGridSelector'; +import { argsEqual } from '../hooks/utils/useGridSelector'; type CacheKey = { id: number }; @@ -328,7 +328,7 @@ export const createSelectorMemoizedV8: CreateSelectorFunctionV8 = (...args: any) const cacheFn = cacheArgs?.get(args); if (cacheArgs && cacheFn) { - if (!areArgsEqual(cacheFn.selectorArgs, selectorArgs)) { + if (!argsEqual(cacheFn.selectorArgs, selectorArgs)) { const reselectArgs = selectorArgs !== undefined ? [...args.slice(0, args.length - 1), () => selectorArgs, args[args.length - 1]] From f673456105aa2be8254a8c1e41e9a6700dc0bd84 Mon Sep 17 00:00:00 2001 From: Bilal Shafi Date: Wed, 6 Nov 2024 12:28:34 +0500 Subject: [PATCH 6/8] Add eslint argsIgnorePattern for derived arguments selector --- .eslintrc.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.eslintrc.js b/.eslintrc.js index d7b85f4b16de..6790094660ea 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -148,6 +148,12 @@ module.exports = { ), }, ], + "@typescript-eslint/no-unused-vars": [ + "warn", + { + "argsIgnorePattern": "^_", + } + ], // TODO move rule into the main repo once it has upgraded '@typescript-eslint/return-await': 'off', 'no-restricted-imports': 'off', From 9b8acecc74320685bfacf0491f9984b775dd6192 Mon Sep 17 00:00:00 2001 From: Bilal Shafi Date: Wed, 6 Nov 2024 12:29:40 +0500 Subject: [PATCH 7/8] Prettier --- .eslintrc.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 6790094660ea..f56e202e584a 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -148,11 +148,11 @@ module.exports = { ), }, ], - "@typescript-eslint/no-unused-vars": [ - "warn", + '@typescript-eslint/no-unused-vars': [ + 'warn', { - "argsIgnorePattern": "^_", - } + argsIgnorePattern: '^_', + }, ], // TODO move rule into the main repo once it has upgraded '@typescript-eslint/return-await': 'off', From 2f4463dac260fa68c6884554a8bf24cdaa8baee4 Mon Sep 17 00:00:00 2001 From: Bilal Shafi Date: Wed, 6 Nov 2024 14:09:04 +0500 Subject: [PATCH 8/8] Update eslint rule --- .eslintrc.js | 6 +-- .../src/components/GridDetailPanels.tsx | 1 - .../src/components/GridPinnedRows.tsx | 1 - .../features/columns/gridColumnsSelector.ts | 37 ++++++++++++++++++- packages/x-data-grid/src/utils/utils.ts | 2 - 5 files changed, 38 insertions(+), 9 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index f56e202e584a..00b62f83a456 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -149,10 +149,8 @@ module.exports = { }, ], '@typescript-eslint/no-unused-vars': [ - 'warn', - { - argsIgnorePattern: '^_', - }, + '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', diff --git a/packages/x-data-grid/src/components/GridDetailPanels.tsx b/packages/x-data-grid/src/components/GridDetailPanels.tsx index 54508a64491f..f8222f0a8f73 100644 --- a/packages/x-data-grid/src/components/GridDetailPanels.tsx +++ b/packages/x-data-grid/src/components/GridDetailPanels.tsx @@ -4,7 +4,6 @@ export interface GridDetailPanelsProps { virtualScroller: VirtualScroller; } -// eslint-disable-next-line @typescript-eslint/no-unused-vars export function GridDetailPanels(_: GridDetailPanelsProps) { return null; } diff --git a/packages/x-data-grid/src/components/GridPinnedRows.tsx b/packages/x-data-grid/src/components/GridPinnedRows.tsx index a1437fb62dbe..46c5f0b57920 100644 --- a/packages/x-data-grid/src/components/GridPinnedRows.tsx +++ b/packages/x-data-grid/src/components/GridPinnedRows.tsx @@ -5,7 +5,6 @@ export interface GridPinnedRowsProps { virtualScroller: VirtualScroller; } -// eslint-disable-next-line @typescript-eslint/no-unused-vars export function GridPinnedRows(_: GridPinnedRowsProps) { return null; } diff --git a/packages/x-data-grid/src/hooks/features/columns/gridColumnsSelector.ts b/packages/x-data-grid/src/hooks/features/columns/gridColumnsSelector.ts index f252388bc18f..9a6e12b0c4c1 100644 --- a/packages/x-data-grid/src/hooks/features/columns/gridColumnsSelector.ts +++ b/packages/x-data-grid/src/hooks/features/columns/gridColumnsSelector.ts @@ -1,4 +1,8 @@ -import { createSelector, createSelectorMemoized } from '../../../utils/createSelector'; +import { + createSelector, + createSelectorMemoized, + createSelectorMemoizedV8, +} from '../../../utils/createSelector'; import { GridStateCommunity } from '../../../models/gridStateCommunity'; import { GridColumnLookup, @@ -198,3 +202,34 @@ export const gridHasColSpanSelector = createSelectorMemoized( gridColumnDefinitionsSelector, (columns) => columns.some((column) => column.colSpan !== undefined), ); + +type Args = { + baseSelectorArgs: string; + derivedSelectorArgs: string; +}; + +// First selector that requires arguments +const baseSelector = createSelectorMemoizedV8( + gridColumnsStateSelector, + (selectorXOutput, args: Args) => { + // compute sub-state and return + return `${selectorXOutput} ${args.baseSelectorArgs}`; + }, +); + +// Another selector that derives `baseSelector` and also requires some more arguments +const derivedSelector = createSelectorMemoizedV8(baseSelector, (baseSelectorOutput, args: Args) => { + // This function doesn't itself need `baseSelectorArgs` but it should still make them required + // so that `baseSelectorOutput` is computed properly + return `${baseSelectorOutput} ${args.baseSelectorArgs}`; +}); + +// Another selector that derives `derivedSelector` and doesn't require any arguments +export const derivedSelector2 = createSelectorMemoizedV8( + derivedSelector, + (derivedSelectorOutput, _args: Args) => { + // This function doesn't require any args but it should still `baseSelectorArgs` and `derivedSelectorArgs` make them required + // so that the underlying selectors are computed properly + return `${derivedSelectorOutput}`; + }, +); diff --git a/packages/x-data-grid/src/utils/utils.ts b/packages/x-data-grid/src/utils/utils.ts index 097ed683d9b2..623213bd3b49 100644 --- a/packages/x-data-grid/src/utils/utils.ts +++ b/packages/x-data-grid/src/utils/utils.ts @@ -209,11 +209,9 @@ export function deepClone(obj: Record) { 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 */