[DataGrid] getCellAggregationResult: handle null rowNode case

[romgrk]

Closes #9876

Remove a ! and handle the null case.

I find that ! often ends up causing bugs down the line. I wonder if there is a set of rules we could enfore for its usage that would protect us more from bugs.

Before: https://codesandbox.io/s/frosty-burnell-smdsgs?file=/Demo.tsx
After: https://codesandbox.io/s/vibrant-wu-c9gjj7?file=/Demo.tsx

[mui-bot]

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9915--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	336.6	502.4	390.2	408.26	68.222
Sort 100k rows	ms	569.3	979.6	824.6	803.08	150.713
Select 100k rows	ms	143.2	243.1	181	190.34	33.842
Deselect 100k rows	ms	126.8	259.1	175.5	181.6	47.424

Generated by 🚫 dangerJS against 3a8ab6e

[MBilalShafi]

I find that ! often ends up causing bugs down the line. I wonder if there is a set of rules we could enfore for its usage that would protect us more from bugs.

Agreed we should reduce its usage where possible, one justified use is when we set a default value for a function argument but TS infers it to be possibly null or undefined.

[cherniavskii]

Should we enable this rule?
https://typescript-eslint.io/rules/no-non-null-assertion/

[jon-spover]

Thanks for fixing this! Quick question: how can I find which version this change is released in so I can bump the package.json in our client app?

[romgrk]

We release each week at the end of the week. Unreleased yet.

[jon-spover]

Sounds good. Thanks again for the fix!

[oliviertassinari]

Should we enable this rule? https://typescript-eslint.io/rules/no-non-null-assertion/

I think there is one case where using ! is better: a ref should always be defined when access is in React effect. If it's null, something else is broken. It's only because we are using ! that this surface and we have a chance to fix the underlying problem. An illustration #9965.

I don't know how this rule behaves, but I think it would be great to retain this ! use cases for ref & React effect.

---

In the case of this PR, how is it possible for apiRef.current.getRowNode called inside a useCallback to not be defined? Does the grid unsubscribe to an event at the wrong time? Does the grid has an unstable useRef definition? Else?

[romgrk]

In the case of this PR, how is it possible for apiRef.current.getRowNode called inside a useCallback to not be defined? Does the grid unsubscribe to an event at the wrong time? Does the grid has an unstable useRef definition? Else?

Since #9037, useGridSelector uses its selector to check if it should re-render (aka update its internal state). So if a row is deleted, useGridSelector is still going to do one more reactive check before being unmounted, because reactive checks are done synchronously with grid state updates. This means the API getRow and getRowNode functions need to return an empty value rather than throwing up when there is no row for an ID.
There are other logic changes in #9254 and other performance PRs that made it a requirement to get an empty value rather than an error for the API functions. This wasn't a breaking change because all the typings already included the possibility of an empty value, it's just that in practice the functions were throwing rather than returning an empty value.

[oliviertassinari]

@romgrk I had a quick look at the error to wrap my head around what's going on. The root issue I can find is different.

As far as I can see, the error happens when getCellAggregationResult is called with an undefined id, which is not compatible with the type GridRowId. Because the id is undefined, getRowNode returns null.

Why is the id undefined? This seems to solve this issue:

diff --git a/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/wrapColumnWithAggregation.tsx b/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/wrapColumnWithAggregation.tsx
index d724dcd49..6f0b91b22 100644
--- a/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/wrapColumnWithAggregation.tsx
+++ b/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/wrapColumnWithAggregation.tsx
@@ -1,6 +1,7 @@
 import * as React from 'react';
 import { GridColDef, GridFilterOperator, GridRowId } from '@mui/x-data-grid-pro';
 import { GridBaseColDef, tagInternalFilter } from '@mui/x-data-grid-pro/internals';
+import { GRID_ID_AUTOGENERATED } from '@mui/x-data-grid/internals';
 import { GridApiPremium } from '../../../models/gridApiPremium';
 import {
   GridAggregationCellMeta,
@@ -154,7 +155,8 @@ const getWrappedFilterOperators: ColumnPropertyWrapper<'filterOperators'> = ({
               return null;
             }
             return (value, row, column, api) => {
-              if (getCellAggregationResult(row.id, column.field) != null) {
+              const id = GRID_ID_AUTOGENERATED in row ? row[GRID_ID_AUTOGENERATED] : row.id;
+              if (getCellAggregationResult(id, column.field) != null) {
                 return true;
               }
               return filterFn(value, row, column, api);
@@ -203,10 +205,7 @@ export const wrapColumnWithAggregationValue = ({
     field: string,
   ): GridAggregationLookup[GridRowId][string] | null => {
     let cellAggregationPosition: GridAggregationPosition | null = null;
-    const rowNode = apiRef.current.getRowNode(id);
-    if (!rowNode) {
-      return null;
-    }
+    const rowNode = apiRef.current.getRowNode(id)!;

     if (rowNode.type === 'group') {
       cellAggregationPosition = 'inline';

[romgrk]

Ah, good catch. I think we also need to add props.getRowId?.(row) before row.id. We should standardize our way of accessing the row ID, I'm pretty sure we have a few unreported bugs related to row ID usage.