Open j3ski-bioraptor opened 3 weeks ago
@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 ?
@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?
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?
@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 This is based on the assumption that the
value
type corresponds to the column type, correct? If so, it's not really helpful forvalueGetter
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!
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:this needs to be changed to
However in 7.x the generic type of
value
here defaults tonever
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/C4TwDgpgBAQgTgewNYQHYDECuqDGwCWCqUAvFABQCGcA5gFxSoQBuEcAlKQHxTML4ATAFBCcRAM7AoAM1QN4yNFlwEipCgG9gESQF9OJHhqFRTUManEIANhAB01hDXLbJ7IbqAI've tried running codemods, but they did nothing.
BTW. I would like to know why
never
was chosen here instead ofunknown
, 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