[RFC] Remove the columnTypes prop

[oliviertassinari]

I don't understand the use case for the columnTypes prop. I think that we can remove the prop and use the spreading approach. From what I understand the idea originated from ag-Grid as I can't find this pattern in any other prior-art data grid.

Mind that 4 years ago, the shape of EMACScript was a lot different. Their grid seems to inherit this legacy constraint. I have documented the spreading alternative, which seems to be simpler: https://material-ui.com/components/data-grid/columns/#custom-column-types

[dtassone]

If you create a column type like price, it's easier to use with this prop. Of course you can spread the object. But it's just more convenient having it in.
If an enterprise wraps the grid with a bunch of col types. Then their consumer will just set the col type to get the desired column config
It's used internally for string number, boolean, checkbox column...

[oliviertassinari]

From what I understand, in both cases, it's a one line change. The advantage I see with the spreading approach is that it removes magic. Developers have a higher level of control and debuggability.

If an enterprise wraps the grid with a bunch of col types. Then their consumer will just set the col type to get the desired column config

The alternative with the spreading approach would be to import the correct type and spread it.

[oliviertassinari]

Maybe we can keep the feature, not document it, and see if developers ask for it? Then, in X months we re-evaluate.

[dtassone]

Up to you. I would keep it as I see value in it and as it's used internally.
I've come across the use case several times. Ag grid has it.

Why not document it? It's a feature and it doesn't hurt the grid, just give another option.

[oliviertassinari]

Why not document it?

An advantage of not documenting, making it hidden to the developer, it to allow feedback:
If there is pain that isn't solved, we might hear about it (they miss this feature). If there is pain that is solved (they use the feature), we will very likely not hear about it. If there is no pain, we can simplify the exposed API, reducing mental overhead.

I think that the fewer APIs we have to expose and get away with, the better. If this is one case that can allow simplifying the API, I think that we can consider it, we don't need to rush in any direction hence my proposal to wait.

[flaviendelangle]

@oliviertassinari @m4theushw @DanailH I would like to update this discussion.

I am not a fan of this property which add a level of complexity that I think could be avoided for now.
I prefer to have a code a little more verbose on the user application than have this concept of extendType and columnTypes. The only usecase we currently have is the price and could easily be built without it.
But if the team thinks it is useful fine for me.

I think we should choose between the following options

• We remove columnTypes from the documentation and rename it UNSTABLE_columnTypes, we can then postpone the decision of removing it during a later version of v5
• We remove columnTypes from the codebase as proposed by @oliviertassinari
• We keep columnTypes and close this issue
• We keep columnTypes until v6 to reconsider then and let this issue open

[cherniavskii]

I think I just found a good use case for columnTypes prop.
Recently I've been working on data grid + pickers integration (#5053) and thinking what would be the next step towards out of the box support for pickers in data grid.
One solution that I really like is to release a new package (let's call it @mui/x-data-grid-pickers) and the integration would look like the following:

import { DataGrid } from '@mui/x-data-grid';
import { createDateColumnType, createDateTimeColumnType } from `@mui/x-data-grid-pickers`;
import { AdapterDateFns } from '@mui/x-date-pickers/AdapterDateFns';
import locale from 'date-fns/locale/en-US';

const dateAdapter = new AdapterDateFns({ locale });

const dateColumnType = createDateColumnType({ dateAdapter });
const dateTimeColumnType = createDateTimeColumnType({ dateAdapter });

// ...

<LocalizationProvider dateAdapter={AdapterDateFns} adapterLocale={locale}>
  <DataGrid
    columnTypes={{
      date: dateColumnType,
      dateTime: dateTimeColumnType,
    }}
    rows={rows}
    columns={columns}
  />
</LocalizationProvider>

Upsides of this approach:

• it doesn't touch column definitions at all, so it can be easily used for all DataGrid instances in the project without messing up with column types
• we do not bundle pickers with the grid

Downsides:

• ? I don't see any

cc @flaviendelangle @alexfauquette

[alexfauquette]

I assume createDateColumnType({ dateAdapter }); return the default properties of a column with type date.

So the same thing could be done as follow

const dateTypeColumn = createDateColumnType({ dateAdapter });

const columns = [
  {
    fied: 'birthDate',
    ...dateTypeColumn,
  }
]

[cherniavskii]

I assume createDateColumnType({ dateAdapter }); return the default properties of a column with type date.

So the same thing could be done as follow

const dateTypeColumn = createDateColumnType({ dateAdapter });

const columns = [
  {
    fied: 'birthDate',
    ...dateTypeColumn,
  }
]

Yes, but if you have a lot of DataGrid instances over your project, you'll have to remember to pass it in every data/dateTime column definition. With columnTypes you can do this once and for all.

[flaviendelangle]

A few arguments against keeping custom column types

1. They will be a pain if we decide at some point to have the plugins responsible for there default GridColDef properties (instead of defining them in GRID_STRING_COL_DEF)

2. They are a pain for features like aggregation where we enable some aggregation functions for some column types because when creating a custom column type, you need to re-define every aggregation function to also register the new type

So I think that we should either create a real notion of parent / children in the column type instead of just merging the objects.
Or remove the notion altogether.

Right now the approach a way to basic to be viable in my opinion

[Janpot]

Another argument from toolpad point of view in favor of keeping the column types (I just learned about the existence of columnTypes property). We store column configurations serverside, they need to be serializable to be persisted. The spread method doesn't work for us in that case as we need to be able to define things like valueGetter, which is not serializable to JSON. We ended up mimicking what the columnTypes does by preprocessing the serializable columns and looking up definition from a COLUMN_TYPES dictionary.

[rgavrilov]

I've been using DataTables.net for a bit and grew to expect this option, it was surprising when I wasn't able to find it. Spreading does eliminate the magic, however, it introduces inconsistency in the interface - a new developer who sees type: 'date' will expect that any other app-specific types (e.g. `type: 'gamescore') would be available using the same pattern. I haven't tried but it may not be too hard to override ColDef type in a project to include additional custom values for type field for intellisense. Gimi!

[DanailH]

It doesn't seem we have a decision on this. It looks like the prop does have useful cases.

[cherniavskii]

I went through the discussion and related issues to gather some information to help us decide how to proceed with columnTypes.
Here's what I've found so far:

Use cases where the columnTypes prop is useful:

• It’s a workaround for column type "detailPanelToggle" not supported #4769
  
We don’t have other workarounds for now.
• It’s convenient for Data Grid and Pickers integration - see [RFC] Remove the columnTypes prop #242 (comment)

Issues with the columnTypes prop:

1. @flaviendelangle: They will be a pain if we decide at some point to have the plugins responsible for there default GridColDef properties (instead of defining them in GRID_STRING_COL_DEF)



   If GRID_STRING_COL_DEF is gone, would it be possible to create a custom column type with an object spread?

2. @flaviendelangle: They are a pain for features like aggregation where we enable some aggregation functions for some column types because when creating a custom column type, you need to re-define every aggregation function to also register the new type



   Potential solution: use both type and extendType in canColumnHaveAggregationFunction:

   mui-x/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/gridAggregationUtils.ts

   Line 63 in 4891eaf

   return aggregationFunction.columnTypes.includes(column.type!);

   This would allow defining a custom column type that inherits the aggregation functions of the extendType.

3. Types - no autocomplete suggestion for custom column types in GridColDef[‘type’].

   It turned out that there are no autocomplete suggestions for the built-in column types either - see https://codesandbox.io/s/sparkling-butterfly-vm2sht?file=/demo.tsx

   This is due to this TS behavior: Literal String Union Autocomplete microsoft/TypeScript#29729
   For the custom column types, we can expose an interface that users can override using module augmentation.

[cherniavskii]

The conclusion is to remove the columnTypes prop, since we don't really see strong arguments to keep it.
We can add it back later considering the potential issues with it if the community requests it.