Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] Change GridColDef methods signatures #11573

Merged
merged 27 commits into from
Feb 1, 2024

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Jan 4, 2024

Closes #10741

Changelog

Breaking changes

  • The signature of GridColDef['valueGetter'] has been changed for performance reasons:

    - valueGetter: ({ value, row }) => value,
    + valueGetter: (value, row, column, apiRef) => value,

    The GridValueGetterParams interface has been removed:

    - const customValueGetter = (params: GridValueGetterParams) => params.row.budget;
    + const customValueGetter: GridValueGetterFn = (value, row) => row.budget;
  • The signature of GridColDef['valueFormatter'] has been changed for performance reasons:

    - valueFormatter: ({ value }) => value,
    + valueFormatter: (value, row, column, apiRef) => value,

    The GridValueFormatterParams interface has been removed:

    - const gridDateFormatter = ({ value, field, id }: GridValueFormatterParams<Date>) => value.toLocaleDateString();
    + const gridDateFormatter: GridValueFormatter = (value: Date) => value.toLocaleDateString();
  • The signature of GridColDef['valueSetter'] has been changed for performance reasons:

    - valueSetter: (params) => {
    -   const [firstName, lastName] = params.value!.toString().split(' ');
    -   return { ...params.row, firstName, lastName };
    - }
    + valueSetter: (value, row) => {
    +   const [firstName, lastName] = value!.toString().split(' ');
    +   return { ...row, firstName, lastName };
    +}

    The GridValueSetterParams interface has been removed:

    - const setFullName = (params: GridValueSetterParams) => {
    -   const [firstName, lastName] = params.value!.toString().split(' ');
    -   return { ...params.row, firstName, lastName };
    - };
    + const setFullName: GridValueSetter<Row> = (value, row) => {
    +   const [firstName, lastName] = value!.toString().split(' ');
    +   return { ...row, firstName, lastName };
    + }
  • The signature of GridColDef['valueParser'] has been changed for performance reasons:

    - valueParser: (value, params: GridCellParams) => value.toLowerCase(),
    + valueParser: (value, row, column, apiRef) => value.toLowerCase(),
  • The signature of GridColDef['colSpan'] has been changed for performance reasons:

    - colSpan: ({ row, field, value }: GridCellParams) => (row.id === 'total' ? 2 : 1),
    + colSpan: (value, row, column, apiRef) => (row.id === 'total' ? 2 : 1),
  • The signature of GridColDef['pastedValueParser'] has been changed for performance reasons:

    - pastedValueParser: (value, params) => new Date(value),
    + pastedValueParser: (value, row, column, apiRef) => new Date(value),
  • The signature of GridColDef['groupingValueGetter'] has been changed for performance reasons:

    - groupingValueGetter: (params) => params.value.name,
    + groupingValueGetter: (value: { name: string }) => value.name,

@cherniavskii cherniavskii added breaking change component: data grid This is the name of the generic UI component, not the React module! v7.x labels Jan 4, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 16, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2024
@cherniavskii
Copy link
Member Author

I have measured the performance impact of this PR on quick filtering by measuring the execution time of the applyFilters function after applying the quick filter that with no match (every column gets checked against the filtering value).
It's around 20-30% faster depending on the number of rows and columns.
Here are the detailed results:

next HEAD diff
10k rows, 10 columns (no filter match)
Median (ms) 12.54 9.57 -23.72%
Mean (ms) 13.10 10.13 -22.71%
σ (for 20 runs) 1.64 1.76
100k rows, 29 columns (no filter match)
Median (ms) 883.76 636.38 -27.99%
Mean (ms) 909.49 643.40 -29.26%
σ (for 20 runs) 77.06 22.91

I'm not 100% if it's worth pushing forward, because:

  • most end users won't feel the difference
  • breaking changes will impact almost every user

@romgrk @joserodolfofreitas What do you think?

@cherniavskii cherniavskii marked this pull request as ready for review January 22, 2024 21:23
@romgrk
Copy link
Contributor

romgrk commented Jan 22, 2024

I'm still for these changes because:

  • It's 20-30% here, but they will also affect other parts of the grid wherever we use those functions.
  • Quickfilters aren't usable for larger datasets, 800+ms is horribly long. And I imagine you run those numbers of your (fast) computer.
  • The complexity of the change for users seems quite low.

@cherniavskii cherniavskii changed the title [DataGrid] Change GridColDef methods signature [DataGrid] Change GridColDef methods signatures Jan 24, 2024
Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement 🚀
I think it makes sense to add these changes to the migration guide too

@cherniavskii cherniavskii enabled auto-merge (squash) February 1, 2024 13:26
@cherniavskii cherniavskii merged commit 0c7c674 into mui:next Feb 1, 2024
15 checks passed
@cherniavskii cherniavskii deleted the GridColDef-methods-signatures branch February 1, 2024 14:21
@quintonlucyk
Copy link

Is this documentation up to date? I'm encountering Module '"@mui/x-data-grid-premium"' has no exported member 'GridColDef'.
image

@romgrk
Copy link
Contributor

romgrk commented Apr 3, 2024

Open a new issue for assistance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! performance v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[datagrid] v7 API: update GridColDef methods signatures
5 participants