[datagrid] v7 API: update GridColDef methods signatures

[romgrk]

To be able to optimize the performance of the filter & quickfilter, we need to change the API of those two column properties to match the API used for getApplyFilterFn.

Search keywords:

[cherniavskii]

Looking at the GridColDef properties, here's a potential list of functions for the signature change:

• valueGetter
• groupingValueGetter
• valueFormatter
• valueSetter
• valueParser
• pastedValueParser

If I understand correctly, only the first 3 could improve performance, as the rest of them are not called on every row.
For valueSetter it makes sense to change its signature for consistency.
We could keep the valueParser and pastedValueParser signatures unchanged though.
@romgrk What do you think?

[romgrk]

Yes sounds good. I'm open to either option for the ones that don't affect performance, though if we can make it consistent it would be nice.

renderCell might be worth examining, it can also be called a lot.

For pastedValueParser, could it affect perfomance if the user pastes a large amount of data?

[cherniavskii]

For pastedValueParser, could it affect perfomance if the user pastes a large amount of data?

Good point, in case of a large amount of data this could affect performance.
It's the same for valueSetter and valueParser because they're called during clipboard paste if defined.

[cherniavskii]

renderCell might be worth examining, it can also be called a lot.

Yeah, with renderCell it will be a bit harder.
We could change the signature like this:

- renderCell?: (params: GridRenderCellParams<R, V, F>) => React.ReactNode;
+ renderCell?: (
+   value: V,
+   row: R,
+   column: GridColDef<R, V, F>,
+   apiRef: React.MutableRefObject<GridApiCommunity>,
+ ) => React.ReactNode;

But the DX is not great if you need to get something you previously got in params, take this as an example:

mui-x/docs/data/data-grid/column-definition/RenderCellGrid.tsx

Lines 6 to 7 in a6f1170

	function RenderDate(props: GridRenderCellParams<any, Date>) {
	const { hasFocus, value } = props;

Is this recommended?

renderCell: (value, row, column, apiRef) => {
  const rowId = apiRef.current.getRowId(row);
  const cellParams = apiRef.current.getCellParams(rowId, column.field);
  const { hasFocus } = cellParams;
}

This feels like a much worse DX to me.
Is there anything we can do to make it better? Maybe pass cellParams that will evaluate lazily?

Another thing with renderCell is that in DataGridPremium we also pass aggregation metadata along with cell params:

mui-x/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/wrapColumnWithAggregation.tsx

Line 114 in a6f1170

return renderCell({ ...params, aggregation: aggregationMeta });

We will have to pass it as a fifth parameter to renderCell:

renderCell?: (
  value: V,
  row: R,
  column: GridColDef<R, V, F>,
  apiRef: React.MutableRefObject<GridApiCommunity>,
+ aggregation?: GridAggregationCellMeta
) => React.ReactNode;

I struggle to rationalize these changes on renderCell (and renderEditCell) because of the pros/cons ratio:

Pros:

1. Better performance
2. API consistency with other GridColDef properties

Cons:

1. The DX feels worse
2. Those are breaking changes that our users will have to deal with
3. This requires a decent effort on our side to make these changes

What do you think?
cc @romgrk @MBilalShafi

[romgrk]

Yes ok let's leave it out for now. I'll benchmark the potential impact before bringing that one up again.

Sidenote, we're passing api instead of apiRef in the cell params, but our selectors require apiRef. We should standardize in one way of passing the API around.

[cherniavskii]

I propose the following scope then:

1. Change the signatures:

• valueGetter

- valueGetter?: (params: GridValueGetterParams<R, any>) => V;
+ valueGetter?: (value: V, row: R, column: GridColDef<R, V, F>, apiRef: React.MutableRefObject<GridApiCommunity>) => V;

• groupingValueGetter

- groupingValueGetter?: (params: GridGroupingValueGetterParams<R, V>) => GridKeyValue | null | undefined;
+ groupingValueGetter?: (value: V, row: R, column: GridColDef<R, V, F>, apiRef: React.MutableRefObject<GridApiPremium>) => GridKeyValue | null | undefined;

• valueFormatter

- valueFormatter?: (params: GridValueFormatterParams<V>) => F;
+ valueFormatter?: (value: V, row: R, column: GridColDef<R, V, F>, apiRef: React.MutableRefObject<GridApiCommunity>) => F;

• valueSetter

- valueSetter?: (params: GridValueSetterParams<R, V>) => R;
+ valueSetter?: (value: V, row: R, column: GridColDef<R, V, F>, apiRef: React.MutableRefObject<GridApiCommunity>) => R;

• valueParser

- valueParser?: (value: F | undefined, params?: GridCellParams<R, V, F>) => V;
+ valueParser?: (value: F | undefined, row: R, column: GridColDef<R, V, F>, apiRef: React.MutableRefObject<GridApiCommunity>) => V;

• pastedValueParser

- pastedValueParser?: (value: string, params: GridCellParams<R, V, F>) => V | undefined;
+ pastedValueParser?: (value: string, row: R, column: GridColDef<R, V, F>, apiRef: React.MutableRefObject<GridApiPremium>) => V | undefined;

2. Pass the apiRef instead of the api:

• GridRenderCellParams
  

  mui-x/packages/grid/x-data-grid/src/models/params/gridCellParams.ts

  Line 86 in ab6fb0e

  api: GridApiCommunity;

• GridRenderEditCellParams
  

  mui-x/packages/grid/x-data-grid/src/models/params/gridCellParams.ts

  Line 108 in ab6fb0e

  api: GridApiCommunity;

• GridSortCellParams:
  

  mui-x/packages/grid/x-data-grid/src/models/gridSortModel.ts

  Line 10 in ab6fb0e

  api: any;

[cherniavskii]

@romgrk

Yes ok let's leave it out for now. I'll benchmark the potential impact before bringing that one up again.

Did you mean leaving out only the renderCell or the overall effort of changing GridColDef methods signatures?

[romgrk]

Just renderCell. The other ones scale with the number of rows (except valueSetter but consistency with valueGetter is important), whereas that one scales with the number of rows rendered in the viewport. The only situations where it could have more impact is during high-speed scrolling.