mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.52k stars 1.31k forks source link

[RFC] Remove the `columnTypes` prop #242

Closed oliviertassinari closed 1 year ago

oliviertassinari commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

dtassone commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

@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

cherniavskii commented 2 years ago

I think I just found a good use case for columnTypes prop. Recently I've been working on data grid + pickers integration (https://github.com/mui/mui-x/pull/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:

Downsides:

cc @flaviendelangle @alexfauquette

alexfauquette commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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

cherniavskii commented 1 year ago

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:

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:

    https://github.com/mui/mui-x/blob/4891eafe516f698fbe3689ba6a3a6c78ed069e85/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/gridAggregationUtils.ts#L63

    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: https://github.com/microsoft/TypeScript/issues/29729 For the custom column types, we can expose an interface that users can override using module augmentation.

cherniavskii commented 1 year ago

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.