[data grid] Why is GridValueFormatter defaulting the value to never?

[j3ski-bioraptor]

The problem in depth

I have an issue while migrating from 6.x to 7.x.

In a lot of places I have the valueFormatter method configured as such:

{
  // other column configuration
  valueFormatter: ({ value }) => someValueFormatter(value)
}

this needs to be changed to

{
  // other column configuration
  valueFormatter: value => someValueFormatter(value)
}

However in 7.x the generic type of value here defaults to never and Typescript will not complain when attempting to destructure this:
https://www.typescriptlang.org/play/?noUncheckedIndexedAccess=true&noUnusedLocals=true&noUnusedParameters=true&exactOptionalPropertyTypes=true&suppressImplicitAnyIndexErrors=true&noPropertyAccessFromIndexSignature=true&noImplicitOverride=true&noFallthroughCasesInSwitch=true#code/C4TwDgpgBAQgTgewNYQHYDECuqDGwCWCqUAvFABQCGcA5gFxSoQBuEcAlKQHxTML4ATAFBCcRAM7AoAM1QN4yNFlwEipCgG9gESQF9OJHhqFRTUManEIANhAB01hDXLbJ7IbqA

I've tried running codemods, but they did nothing.

BTW. I would like to know why never was chosen here instead of unknown, which would at least make it explode.

Your environment

`npx @mui/envinfo`

  System:
    OS: macOS 12.6
  Binaries:
    Node: 18.10.0 - ~/.nvm/versions/node/v18.10.0/bin/node
    npm: 8.19.2 - ~/.nvm/versions/node/v18.10.0/bin/npm
    pnpm: 7.8.0 - /opt/homebrew/bin/pnpm
  Browsers:
    Chrome: 124.0.6367.91
    Edge: 124.0.2478.51
    Safari: 16.0
  npmPackages:
    @emotion/react: ^11.11.1 => 11.11.1
    @emotion/styled: ^11.11.0 => 11.11.0
    @mui/base:  5.0.0-beta.13
    @mui/core-downloads-tracker:  5.15.15
    @mui/icons-material: ^5.14.7 => 5.14.7
    @mui/lab: ^5.0.0-alpha.142 => 5.0.0-alpha.142
    @mui/material: ^5.15.15 => 5.15.15
    @mui/private-theming:  5.15.14
    @mui/styled-engine:  5.15.14
    @mui/system:  5.15.15
    @mui/types:  7.2.14
    @mui/utils:  5.15.14
    @mui/x-data-grid:  7.3.0
    @mui/x-data-grid-pro: ^7.3.0 => 7.3.0
    @mui/x-date-pickers: ^6.16.3 => 6.16.3
    @mui/x-license:  7.2.0
    @types/react: ^18.0.15 => 18.0.15
    react: ^18.2.0 => 18.2.0
    react-dom: ^18.2.0 => 18.2.0
    typescript: ^5.1.6 => 5.1.6.

tsconfig

{
  "compilerOptions": {
    "jsx": "preserve",
    "target": "ESNext",
    "module": "esnext",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "skipLibCheck": true,
    "lib": ["dom", "dom.iterable", "esnext"],
    "allowJs": true,
    "allowSyntheticDefaultImports": true,
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "typeRoots": ["node_modules/@types"],
    "types": ["vite/client"],
    "baseUrl": "src"
  },
  "include": ["src"]
}

Search keywords: GridValueFormatter
Order ID: 86856

[michelengelen]

@j3ski-bioraptor thanks for raising this.
Is there any reason we are not using unknown here as well (although I did not have a problem with that yet).

Here is diff for changing the valueFormatter value type to unknown:

diff --git a/packages/x-data-grid/src/models/colDef/gridColDef.ts b/packages/x-data-grid/src/models/colDef/gridColDef.ts
index 36d7619a5..f22ee3291 100644
--- a/packages/x-data-grid/src/models/colDef/gridColDef.ts
+++ b/packages/x-data-grid/src/models/colDef/gridColDef.ts
@@ -57,7 +57,7 @@ export type GridValueFormatter<
   R extends GridValidRowModel = GridValidRowModel,
   V = any,
   F = V,
-  TValue = never,
+  TValue = unknown,
 > = (
   value: TValue,
   row: R,

Any objections @mui/xgrid ?

[cherniavskii]

@j3ski-bioraptor

BTW. I would like to know why never was chosen here instead of unknown, which would at least make it explode.

In v6 of the Data Grid, the value was typed as any.
Initially, I kept the same type, but when migrating to v7 TS wouldn't complain about it:

// TS should complain here, the first argument in v7 is the `value` itself, not the `params` object
valueFormatter: (params) => params.value,

The decision to use never was to get a TS error in the use case above.
It's not perfect though, because TS doesn't show the error if you destructure params.

We didn't use unknown because it's impossible to override on the valueFormatter level.
With never you can do this:

valueFormatter: (value: string, row) => value.toUpperCase(),

It's a trade-off that made sense to me at that time.
What do you think?

[michelengelen]

OK, @cherniavskii this took me a while, but I think I might have a solution for this:

diff --git a/packages/x-data-grid/src/models/colDef/gridColDef.ts b/packages/x-data-grid/src/models/colDef/gridColDef.ts
index 36d7619a5..25066688c 100644
--- a/packages/x-data-grid/src/models/colDef/gridColDef.ts
+++ b/packages/x-data-grid/src/models/colDef/gridColDef.ts
@@ -23,6 +23,21 @@ export type GridAlignment = 'left' | 'right' | 'center';

 export type ValueOptions = string | number | { value: any; label: string } | Record<string, any>;

+type ValueMap = {
+  number: number;
+  string: string;
+  date: Date;
+  dateTime: Date;
+  boolean: boolean;
+  actions: never;
+  singleSelect: ValueOptions;
+  custom: never;
+};
+
+type ValueBasedType<T extends GridColType | undefined> = T extends GridColType
+  ? ValueMap[T]
+  : never;
+
 /**
  * Value that can be used as a key for grouping rows
  */
@@ -170,35 +185,10 @@ export interface GridBaseColDef<R extends GridValidRowModel = GridValidRowModel,
    * @returns {GridComparatorFn<V>} The comparator function to use.
    */
   getSortComparator?: (sortDirection: GridSortDirection) => GridComparatorFn<V> | undefined;
-  /**
-   * The type of the column.
-   * @default 'string'
-   * @see See {@link https://mui.com/x/react-data-grid/column-definition/#column-types column types docs} for more details.
-   */
-  type?: GridColType;
   /**
    * Allows to align the column values in cells.
    */
   align?: GridAlignment;
-  /**
-   * Function that allows to get a specific data instead of field to render in the cell.
-   */
-  valueGetter?: GridValueGetter<R, V, F>;
-  /**
-   * Function that allows to customize how the entered value is stored in the row.
-   * It only works with cell/row editing.
-   * @returns {R} The row with the updated field.
-   */
-  valueSetter?: GridValueSetter<R, V, F>;
-  /**
-   * Function that allows to apply a formatter before rendering its value.
-   */
-  valueFormatter?: GridValueFormatter<R, V, F>;
-  /**
-   * Function that takes the user-entered value and converts it to a value used internally.
-   * @returns {V} The converted value to use internally.
-   */
-  valueParser?: GridValueParser<R, V, F>;
   /**
    * Class name that will be added in cells for that column.
    */
@@ -289,65 +279,131 @@ export interface GridBaseColDef<R extends GridValidRowModel = GridValidRowModel,
    * @default 1
    */
   colSpan?: number | GridColSpanFn<R, V, F>;
-}
-
-/**
- * Column Definition interface used for columns with the `actions` type.
- * @demos
- *   - [Special column properties](/x/react-data-grid/column-definition/#special-properties)
- */
-export interface GridActionsColDef<R extends GridValidRowModel = any, V = any, F = V>
-  extends GridBaseColDef<R, V, F> {
   /**
    * The type of the column.
-   * @default 'actions'
-   */
-  type: 'actions';
-  /**
-   * Function that returns the actions to be shown.
-   * @param {GridRowParams} params The params for each row.
-   * @returns {React.ReactElement<GridActionsCellItemProps>[]} An array of [[GridActionsCell]] elements.
+   * @default 'string'
+   * @see See {@link https://mui.com/x/react-data-grid/column-definition/#column-types column types docs} for more details.
    */
-  getActions: (params: GridRowParams<R>) => React.ReactElement<GridActionsCellItemProps>[];
+  type?: GridColType;
 }

-/**
- * Column Definition interface used for columns with the `singleSelect` type.
- * @demos
- *   - [Special column properties](/x/react-data-grid/column-definition/#special-properties)
- */
-export interface GridSingleSelectColDef<R extends GridValidRowModel = any, V = any, F = V>
-  extends GridBaseColDef<R, V, F> {
+export interface GridValueMethods<
+  T extends GridColType,
+  R extends GridValidRowModel = GridValidRowModel,
+  V = any,
+  F = V,
+> {
   /**
-   * The type of the column.
-   * @default 'singleSelect'
+   * Function that allows to get a specific data instead of field to render in the cell.
    */
-  type: 'singleSelect';
+  valueGetter?: GridValueGetter<R, V, F, ValueBasedType<T>>;
   /**
-   * To be used in combination with `type: 'singleSelect'`. This is an array (or a function returning an array) of the possible cell values and labels.
+   * Function that allows to customize how the entered value is stored in the row.
+   * It only works with cell/row editing.
+   * @returns {R} The row with the updated field.
    */
-  valueOptions?: Array<ValueOptions> | ((params: GridValueOptionsParams<R>) => Array<ValueOptions>);
+  valueSetter?: GridValueSetter<R, V, F>;
   /**
-   * Used to determine the label displayed for a given value option.
-   * @param {ValueOptions} value The current value option.
-   * @returns {string} The text to be displayed.
+   * Function that allows to apply a formatter before rendering its value.
    */
-  getOptionLabel?: (value: ValueOptions) => string;
+  valueFormatter?: GridValueFormatter<R, V, F, ValueBasedType<T>>;
   /**
-   * Used to determine the value used for a value option.
-   * @param {ValueOptions} value The current value option.
-   * @returns {string} The value to be used.
+   * Function that takes the user-entered value and converts it to a value used internally.
+   * @returns {V} The converted value to use internally.
    */
-  getOptionValue?: (value: ValueOptions) => any;
+  valueParser?: GridValueParser<R, V, F>;
 }

+/**
+ * Column Definition interface used for columns with the `actions` type.
+ * @demos
+ *   - [Special column properties](/x/react-data-grid/column-definition/#special-properties)
+ */
+export type GridActionsColDef<R extends GridValidRowModel = any, V = any, F = V> = GridBaseColDef<
+  R,
+  V,
+  F
+> &
+  GridValueMethods<'actions', R, V, F> & {
+    /**
+     * The type of the column.
+     * @default 'actions'
+     */
+    type: 'actions';
+    /**
+     * Function that returns the actions to be shown.
+     * @param {GridRowParams} params The params for each row.
+     * @returns {React.ReactElement<GridActionsCellItemProps>[]} An array of [[GridActionsCell]] elements.
+     */
+    getActions: (params: GridRowParams<R>) => React.ReactElement<GridActionsCellItemProps>[];
+  };
+
+/**
+ * Column Definition interface used for columns with the `singleSelect` type.
+ * @demos
+ *   - [Special column properties](/x/react-data-grid/column-definition/#special-properties)
+ */
+export type GridSingleSelectColDef<
+  R extends GridValidRowModel = any,
+  V = any,
+  F = V,
+> = GridBaseColDef<R, V, F> &
+  GridValueMethods<'singleSelect', R, V, F> & {
+    /**
+     * The type of the column.
+     * @default 'singleSelect'
+     */
+    type: 'singleSelect';
+    /**
+     * To be used in combination with `type: 'singleSelect'`. This is an array (or a function returning an array) of the possible cell values and labels.
+     */
+    valueOptions?:
+      | Array<ValueOptions>
+      | ((params: GridValueOptionsParams<R>) => Array<ValueOptions>);
+    /**
+     * Used to determine the label displayed for a given value option.
+     * @param {ValueOptions} value The current value option.
+     * @returns {string} The text to be displayed.
+     */
+    getOptionLabel?: (value: ValueOptions) => string;
+    /**
+     * Used to determine the value used for a value option.
+     * @param {ValueOptions} value The current value option.
+     * @returns {string} The value to be used.
+     */
+    getOptionValue?: (value: ValueOptions) => any;
+  };
+
+export type GridTypedValueColDef<
+  R extends GridValidRowModel = any,
+  V = any,
+  F = V,
+> = GridBaseColDef<R, V, F> &
+  (
+    | ({
+        type?: 'string';
+      } & GridValueMethods<'string', R, V, F>)
+    | ({
+        type: 'number';
+      } & GridValueMethods<'number', R, V, F>)
+    | ({
+        type: 'date' | 'dateTime';
+      } & GridValueMethods<'date', R, V, F>)
+    | ({
+        type: 'boolean';
+      } & GridValueMethods<'boolean', R, V, F>)
+    | ({
+        type: 'custom';
+      } & GridValueMethods<'custom', R, V, F>)
+  );
+
 /**
  * Column Definition interface.
  * @demos
  *   - [Column definition](/x/react-data-grid/column-definition/)
  */
 export type GridColDef<R extends GridValidRowModel = any, V = any, F = V> =
-  | GridBaseColDef<R, V, F>
+  | GridTypedValueColDef<R, V, F>
   | GridActionsColDef<R, V, F>
   | GridSingleSelectColDef<R, V, F>;

I guess this could be improved a bit still, but it works as long as you type the columns as we do normally in our docs as well:

WDYT?

[cherniavskii]

@michelengelen This is based on the assumption that the value type corresponds to the column type, correct?
If so, it's not really helpful for valueGetter since it's mostly used when the value type does not match the column type:

{
  field: 'dateString',
  type: 'date',
  // convert string to Date
  valueGetter: (value) => new Date(value),
}

Furthermore, there's no way to override the assumed type if necessary.

I would rather look into the TypeScript builder approach where we can use generic type variables to infer the actual type based on the field

[michelengelen]

@michelengelen This is based on the assumption that the value type corresponds to the column type, correct? If so, it's not really helpful for valueGetter since it's mostly used when the value type does not match the column type:

{
  field: 'dateString',
  type: 'date',
  // convert string to Date
  valueGetter: (value) => new Date(value),
}

So valueGetter should actually use TValue = any and return the correct type based on the column type, right?

So something like this:

export type GridValueGetter<
  R extends GridValidRowModel = GridValidRowModel,
  V = any,
  F = V,
  TValue = any,
> = (
  value: any,
  row: R,
  column: GridColDef<R, V, F>,
  apiRef: React.MutableRefObject<GridApiCommunity>,
) => TValue;

valueGetter?: GridValueGetter<R, V, F, ValueBasedType<T>>;

But this feels somewhat wrong.

Furthermore, there's no way to override the assumed type if necessary.

Overridability can be restored. No problem.

I would rather look into the TypeScript builder approach where we can use generic type variables to infer the actual type based on the field

This is an interesting approach. I have used type inference in the past and it led to some nasty errors at that time. Maybe this got better during the latest typescript releases. I will test this in the codebase!

Thanks for the suggestions!