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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Fix memoized selectors with arguments #15215

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
+100 −11
Open

[DataGrid] Fix memoized selectors with arguments #15215

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
782d560
[DataGrid] Improve v8 selectors
MBilalShafi Oct 30, 2024
5e111b8
Only recompute current selector on args change
MBilalShafi Oct 31, 2024
912e09d
Add shallow comparison for complex arguments
MBilalShafi Oct 31, 2024
1694698
Fix types issue + other updates
MBilalShafi Nov 1, 2024
2c43856
Fix eslint
MBilalShafi Nov 1, 2024
f673456
Add eslint argsIgnorePattern for derived arguments selector
MBilalShafi Nov 6, 2024
c698628
Merge branch 'master' into fix-memoized-argument-selectors
MBilalShafi Nov 6, 2024
9b8acec
Prettier
MBilalShafi Nov 6, 2024
2f4463d
Update eslint rule
MBilalShafi Nov 6, 2024
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
21 changes: 18 additions & 3 deletions packages/x-data-grid/src/hooks/utils/useGridSelector.ts
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

When calling the selector directly, it seems to require an args arg even if additional args are not used by the selector.

@KenanYusuf There isn't a perfect fix for it atm, I've settled on making args an optional argument, which would solve the problem you mentioned but not warn now if a selector requires arguments but isn't passed. To solve this problem I think we'll have to go back to this approach.
I guess the tradeoff is liveable since not providing arguments where required should generate runtime errors for the users to fix them anyways.

Thoughts?
CC @mui/xgrid

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the tradeoff is liveable since not providing arguments where required should generate runtime errors for the users to fix them anyways.

I agree on this.

It may be a separate issue, but given the following selectors:

// This selector has a selector arg
export const gridVisibleColumnDefinitionsSelector = createSelectorMemoizedV8(
  gridColumnDefinitionsSelector,
  gridColumnVisibilityModelSelector,
  gridListColumnSelector,
  (columns, columnVisibilityModel, listColumn, listView?: boolean) => {
    if (listView) {
      return listColumn ? [listColumn] : [];
    }
    return columns.filter((column) => columnVisibilityModel[column.field] !== false);
  },
);

// Uses the previous selector, but does not use the selector arg
export const gridVisibleColumnFieldsSelector = createSelectorMemoizedV8(
  gridVisibleColumnDefinitionsSelector,
  (visibleColumns) => visibleColumns.map((column) => column.field),
);

I am seeing this error when calling the selector function:

Screenshot 2024-11-04 at 14 00 59
Full error 
No overload matches this call.
  Overload 1 of 2, '(apiRef: MutableRefObject<{ state: GridStateCommunity; instanceId: { id: number; }; }>, args?: (GridListColDef & { computedWidth: number; })[] | undefined): string[]', gave the following error.
    Argument of type 'boolean | undefined' is not assignable to parameter of type '(GridListColDef & { computedWidth: number; })[] | undefined'.
      Type 'boolean' is not assignable to type '(GridListColDef & { computedWidth: number; })[]'.
  Overload 2 of 2, '(state: GridStateCommunity, instanceId: { id: number; }): string[]', gave the following error.
    Argument of type 'MutableRefObject<GridPrivateApiCommunity>' is not assignable to parameter of type 'GridStateCommunity'.
      Type 'MutableRefObject<GridPrivateApiCommunity>' is missing the following properties from type 'GridStateCommunity': isRtl, dimensions, rows, visibleRowsLookup, and 19 more.ts(2769)

If I change it to the following, the selector function is typed correctly but the param is unused:

export const gridVisibleColumnFieldsSelector = createSelectorMemoizedV8(
  gridVisibleColumnDefinitionsSelector,
  (visibleColumns, listView?: boolean) => visibleColumns.map((column) => column.field),
);

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 think it's kinda expected. The derived selectors have to have the same arguments (if not more), only then the underlying selectors will be provided with the correct arguments to compute the substate correctly.

gridVisibleColumnDefinitionsSelector requires listView argument and gridVisibleColumnFieldsSelector is derived from it. It should also require listView argument for gridVisibleColumnDefinitionsSelector provide the correct value for the visibleColumns argument.

If multiple selectors require different arguments, we could use an object as argument. For example:

// First selector that requires arguments
const baseSelector = createSelectorMemoized(
	selectorX,
	(selectorXOutput, { baseSelectorArgs }) => {
		// compute sub-state and return
		const baseSelectorOutput = computeValue(selectorXOutput, baseSelectorArgs);
		return baseSelectorOutput;
	}
);

// Another selector that derives `baseSelector` and also requires some more arguments
const derivedSelector = createSelectorMemoized(
	baseSelector,
	(baseSelectorOutput, { baseSelectorArgs, derivedSelectorArgs }) => {
		// This function doesn't itself need `baseSelectorArgs` but it should still make them required
		// so that `baseSelectorOutput` is computed properly
		const derivedSelectorOutput = computeValue(baseSelectorOutput, derivedSelectorArgs);
		return derivedSelectorOutput;
	}
);

// Another selector that derives `derivedSelector` and doesn't require any arguments
const derivedSelector2 = createSelectorMemoized(
	derivedSelector,
	(derivedSelectorOutput, { baseSelectorArgs, derivedSelectorArgs }) => {
		// 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
		const derivedSelector2Output = computeValue(derivedSelectorOutput);
		return derivedSelector2Output;
	}
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it makes sense, but in scenarios where the selector arg is not used, do we just disable @typescript-eslint/no-unused-vars on the line of the unused param?

Copy link
Member Author

@MBilalShafi MBilalShafi Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we could explicitly define the function type to avoid adding an ignore directive and a cleaner code.

How does the following feel:

type BaseSelectorArgs = string;
type DerivedSelectorArgs = string;

type DerivedSelector2OutputFn = (
  derivedSelectorOutput: string,
  args: { derivedSelectorArgs: DerivedSelectorArgs; baseSelectorArgs: BaseSelectorArgs },
) => ReturnType<typeof derivedSelector>;

// Updated `derivedSelector2` from above snippet.
export const derivedSelector2 = createSelectorMemoized(derivedSelector, ((derivedSelectorOutput) => {
  return derivedSelectorOutput;
  }) as DerivedSelector2OutputFn,
);

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels quite verbose doing this for each selector that is derived from another with args.

Ideally, derivedSelector2 in the example above would include the typed selector args in the function signature based on the selectors that are passed to it.

The same way that TS knows here that this first param is GridListColDef[] because it has gridVisibleColumnDefinitionsSelector as the first argument of createSelectorMemoizedV8:

Screenshot 2024-11-05 at 09 36 21

I haven't fully wrapped my head around the types for these selector creator functions, so I don't know if it is possible, but that would be the ideal DX I think.

Copy link
Member Author

@MBilalShafi MBilalShafi Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@KenanYusuf You are right, it's a bit verbose. It aligns with the choice we made in the selectors with arguments PR, i.e. we should explicitly type the arguments when defining the output function.

I think that having the underlying selectors' output function's arguments type detected automatically won't be possible without the two-step generics, as currently it's only possible to infer the function's return type.

We could introduce an ignore pattern for the @typescript-eslint/no-unused-vars rule:

diff --git a/.eslintrc.js b/.eslintrc.js
index d7b85f4b16..6790094660 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 ha
s upgraded
     '@typescript-eslint/return-await': 'off',
     'no-restricted-imports': 'off',

To allow something like following without the need to add the ignore directive.
(Typescript compiler does it automatically.)

const derivedSelector2 = createSelectorMemoized(
	derivedSelector,
	(derivedSelectorOutput, _args: Args) => {
		return doSomething(derivedSelectorOutput)
	}
);

It will be only needed for the selectors that are derived from some selectors with arguments but they do not require any arguments which I assume would be very less use-cases.

How does adding the eslint ignore pattern sound to reduce verbosity for such use cases? Underscore pattern is widely used and shouldn't contribute to reduced readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the context from the initial discussion, makes sense. I think the underscore pattern is a good compromise given the complexities of the alternate solutions. I'm going to continue testing these changes today 👍

Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function applySelectorV8<Api extends GridApiCommon, Args, T>(
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 = <Api extends GridApiCommon, T>(
Expand Down Expand Up @@ -98,7 +98,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 +114,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 +128,27 @@ 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 && !refs.current.equals<typeof args>(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
22 changes: 21 additions & 1 deletion packages/x-data-grid/src/utils/createSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,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 Down Expand Up @@ -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);
Expand Down