[DataGrid] Fix memoized selectors with arguments

[MBilalShafi]

As reported by @KenanYusuf, memoized selectors with arguments weren't working properly, this PR aims to fix that.

Extracted from #15200 to separate from the BC.

[mui-bot]

Deploy preview: https://deploy-preview-15215--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 2f4463d

[KenanYusuf]

Tested on my working branch using memoized v8 selectors and it is working as expected 👍

[KenanYusuf]

@MBilalShafi Spotted an issue with the types for selectors created using createSelectorMemoizedV8:

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

Selector:

export const gridColumnDefinitionsSelectorV8 = createSelectorMemoizedV8(
  gridColumnFieldsSelectorV8,
  gridColumnLookupSelectorV8,
  (allFields, lookup) => allFields.map((field) => lookup[field]),
);

(ignore all the selectors being suffixed with v8, it's for testing purposes)

[MBilalShafi]

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

[KenanYusuf]

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:

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),
);

[MBilalShafi]

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;
	}
);

[KenanYusuf]

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?

[MBilalShafi]

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,
);

[KenanYusuf]

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:

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.

[MBilalShafi]

@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.

[KenanYusuf]

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 👍