[data grid] Aggregation values do not update if column definitions update

[tomdglenn91]

Steps to reproduce

Link to live example: https://stackblitz.com/edit/react-g5fn8t?file=Demo.tsx

Steps:

1. Create a function that generates column definitions based off of external data
2. Create an aggregation for one of those columns that uses external data to calculate the value
3. Alter that external data somehow

Current behavior

The aggregation value does not update reflect the updated rows. The totals correctly say 80 ( 4 * 20), but after 5 seconds, when the value getter changes what that value would be, the aggregation remains at 80.

Expected behavior

The aggregation value should update from 80 to 160 after 5 seconds.

Context

In my real world example, it is to do with exchange rates and selected currencies. When the user selects a new currency, I convert all values to that currency. Aggregation does not keep up. Think something like (v6 api)

const getCols = (selectedCurrency: string, exchangeMap: Record<string, Record<string, number>>) => [
...
{ field: 'convertedValue', valueGetter: ({ row }) => exchangeMap[row.currency][selectedCurrency] * row.value }

When the selected currency changes, this calculated field should update (which it does), but the aggregation value does not!

e.g. 3 rows of 100 GBP, aggregation = 300 GBP. Select usd, each row becomes 150 USD. Aggregation becomes 300 USD.

My workaround is to use state/effect to sync my rows data and recalculate the values that way, so that rows changes rather than columns. This is not ideal however, as my exchange rate info is loaded in parallel and its resulting in slightly clunky code.

Your environment

npx @mui/envinfo

``` System: OS: macOS 14.4.1 Binaries: Node: 18.18.2 - ~/.nvm/versions/node/v18.18.2/bin/node npm: 9.8.1 - ~/.nvm/versions/node/v18.18.2/bin/npm pnpm: 8.14.0 - ~/.nvm/versions/node/v18.18.2/bin/pnpm Browsers: Chrome: 124.0.6367.93 Edge: Not Found Safari: 17.4.1 npmPackages: @emotion/react: ^11.11.1 => 11.11.1 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.33 @mui/core-downloads-tracker: 5.14.13 @mui/icons-material: ^5.8.0 => 5.14.1 @mui/material: ^5.14.13 => 5.14.13 @mui/private-theming: 5.14.13 @mui/styled-engine: 5.14.13 @mui/system: 5.14.13 @mui/types: 7.2.6 @mui/utils: 5.15.6 @mui/x-data-grid: 6.19.3 @mui/x-data-grid-premium: ^6.19.3 => 6.19.3 @mui/x-data-grid-pro: 6.19.3 @mui/x-date-pickers: 6.19.0 @mui/x-date-pickers-pro: ^6.19.0 => 6.19.0 @mui/x-license-pro: 6.10.2 @types/react: ^18.2.79 => 18.2.79 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 styled-components: 5.3.6 => 5.3.6 typescript: 5.0 => 5.0.4 ```

Search keywords: datagrid aggregation columns Order ID: 76489

[michelengelen]

Hey @tomdglenn91 ... sry for the late reply. We are going to look into this!

[michelengelen]

OK, I did some shallow investigation and apparently we do not check if the valueGetter returns a different value when the columns get rehydrated.

This is the codeblock in question:

https://github.com/mui/mui-x/blob/8f03743f6453c9a5e4f01c1f960ce53316924673/packages/x-data-grid-premium/src/hooks/features/aggregation/useGridAggregation.ts#L100-L123

Any idea how we could support checking for a diff in the valueGetter behaviour @romgrk or @MBilalShafi ?

[tomdglenn91]

Thanks for looking into this guys it's gonna save me a huge chunk of messy code if I can just rely on your fancy aggregation stuff. Would this fix go into v7 or would it be possible to also target v6? I've only just managed to shove react 18 upgrade into our product not sure I can swing another major update 😅

[michelengelen]

Thanks for looking into this guys it's gonna save me a huge chunk of messy code if I can just rely on your fancy aggregation stuff. Would this fix go into v7 or would it be possible to also target v6? I've only just managed to shove react 18 upgrade into our product not sure I can swing another major update 😅

I guess this can be backported as well ... but that is for the team to estimate first. From a quick look at it the code is equal to v7. 👍🏼

[MBilalShafi]

@tomdglenn91 Thank you for your patience, your use case is both interesting and a bit unusual. Because in normal circumstances, the columns prop would have a stable reference across multiple renders as stated here.

But since you have to update it often, I'd still recommend memoizing the value to recompute it as less as possible to avoid unnecessary re-rendering and computation cost, I tried to do it in this example: https://codesandbox.io/p/sandbox/clever-torvalds-3mkyrm It will only re-compute when the value of amount updates.

Coming to the original problem, the aggregation algorithm assumes the columns have a stable reference and it only re-applies by default when aggregation rules / aggregation model gets updated for performance reasons, which is good in normal circumstances, but for use-case like this, where we need to re-apply the aggregation on-demand, I think we can expose the internal function applyAggregation to the apiRef which could be used by the devs as required like:

const columns = React.useMemo(() => getColumns(amount), [amount]);

React.useEffect(() => {
  apiRef.current.applyAggregation();
}, [columns]);

I opened up a PR related to this and tested it in this csb example, and it seems to work as expected. Let me know if it makes sense.

[tomdglenn91]

Hey @MBilalShafi thanks for this response. I appreciate it's an odd set of circumstances, when the column definition depends on external factors. We treat valueGetter and renderCell as these blanket calculation fields that sometimes require external fields.

It might not affect us too much but my concern would be out of date values, e.g. before the effect runs, our table is in an invalid state (updated column values but aggregation hasn't kept up).

We decided internally that these fields we have are basically calculated fields and we should enforce that on the row data, not the column data, rather than force a re-calculation.

That being said, I think your api exposure could be useful in similar scenarios we have, so would find the change useful still.