[core] Each feature hook should be responsible for its column pre-processing

[flaviendelangle]

Closes #2814

Extracted from #2725

---

A side effect of using registerColumnsPreProcessing in useGridSelection was that useGridSelection needed to be before useGridColumns.
As a result, it could not use const rowsLookup = useGridSelector(apiRef, gridRowsLookupSelector);
I migrated the selection cleanup when the rows update to an event.
But it can't be GridEvents.rowsSet, otherwise, this cleanup is triggered before the filtering and the rerender it causes seems to be synchronous.

Basically here was the behavior with the exact same code but useGridApiEventHandler(apiRef, GridEvents.rowsSet, removeOutdatedSelection); on the test should not crash when the row is removed during the click

1. We select a line by clicking on it
2. Synchronously after the click, we delete this line with apiRef.current.updateRows
3. GridEvents.rowsSet is published
4. useGridSelection updates its model because the selection row do not exist anymore
5. The DataGrid rerenders => Crashes because the visibleRows state still has the old deleted row
6. useGridSorting and useGridFilter updates their state without the deleted row

I fixed that by adding a GridEvents.visibleRowsSet that useGridFilter triggers after updating the visible rows.

[github-actions]

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

[m4theushw]

As a result, it could not use const rowsLookup = useGridSelector(apiRef, gridRowsLookupSelector);

Why you couldn't use?

I fixed that by adding a GridEvents.visibleRowsSet that useGridFilter triggers after updating the visible rows.

We could check if the row is not null when rendering the rows. It worked with the diff below. Am I missing something?

diff --git a/packages/grid/_modules_/grid/components/GridViewport.tsx b/packages/grid/_modules_/grid/components/GridViewport.tsx
index 863648e1..f5030398 100644
--- a/packages/grid/_modules_/grid/components/GridViewport.tsx
+++ b/packages/grid/_modules_/grid/components/GridViewport.tsx
@@ -71,24 +71,30 @@ export const GridViewport: ViewportType = React.forwardRef<HTMLDivElement, {}>(
         renderState.renderContext.lastColIdx! + 1,
       );
 
-      return renderedRows.map(([id, row], idx) => (
-        <rootProps.components.Row
-          key={id}
-          id={id}
-          row={row}
-          selected={selectionLookup[id] !== undefined}
-          index={renderState.renderContext!.firstRowIdx! + idx}
-          rowHeight={rowHeight}
-          renderedColumns={renderedColumns}
-          firstColumnToRender={renderState.renderContext!.firstColIdx!}
-          cellFocus={cellFocus}
-          cellTabIndex={cellTabIndex}
-          editRowsModel={editRowsState}
-          scrollBarState={scrollBarState}
-          renderState={renderState}
-          {...rootProps.componentsProps?.row}
-        />
-      ));
+      return renderedRows.map(([id, row], idx) => {
+        if (!row) {
+          return null;
+        }
+
+        return (
+          <rootProps.components.Row
+            key={id}
+            id={id}
+            row={row}
+            selected={selectionLookup[id] !== undefined}
+            index={renderState.renderContext!.firstRowIdx! + idx}
+            rowHeight={rowHeight}
+            renderedColumns={renderedColumns}
+            firstColumnToRender={renderState.renderContext!.firstColIdx!}
+            cellFocus={cellFocus}
+            cellTabIndex={cellTabIndex}
+            editRowsModel={editRowsState}
+            scrollBarState={scrollBarState}
+            renderState={renderState}
+            {...rootProps.componentsProps?.row}
+          />
+        );
+      });
     };
 
     return (
diff --git a/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts b/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts
index 3898b485..137c82ff 100644
--- a/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts
+++ b/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts
@@ -325,7 +325,7 @@ export const useGridSelection = (
     [apiRef, props.checkboxSelectionVisibleOnly, props.pagination],
   );
 
-  useGridApiEventHandler(apiRef, GridEvents.visibleRowsSet, removeOutdatedSelection);
+  useGridApiEventHandler(apiRef, GridEvents.rowsSet, removeOutdatedSelection);
   useGridApiEventHandler(apiRef, GridEvents.rowClick, handleRowClick);
   useGridApiEventHandler(
     apiRef,

[flaviendelangle]

@m4theushw

Why you couldn't use?

Because useGridSelection is now before useGridRows in the execution order, so on the 1st render it runs before the row state has even be initialized. Selector in the render of feature hook will tend to be more and more problematic as we add features I think. I was the main trade off to have the state init in the render of the feature hooks (see #2293 (comment) Approach 1 cons)

We could check if the row is not null when rendering the rows. It worked with the diff below. Am I missing something?

I probably works yes
I still find it weird to have the Grid render with an inconsistent state (ie: state.filter.visibleRows containing rows that are not in state.rows.allRows).
My goal here was to keep a logical state update order.

[m4theushw]

I still find it weird to have the Grid render with an inconsistent state (ie: state.filter.visibleRows containing rows that are not in state.rows.allRows).

For now, listening to visibleRowsSet solved the problem. In the long term, I think we should always check if the returned row is not an undefined value. There will be a point where one selector will depend on a bunch of other selectors from other features and these will clean their state in the different occasions.

[m4theushw]

It's worth to give a try on this approach to see where it goes.

[flaviendelangle]

I think we should always check if the returned row is not an undefined value. There will be a point where one selector will depend on a bunch of other selectors from other features and these will clean their state in the different occasions.

OK to improve the reliability of the rows by doing this check.
I think both are not incompatible. We can limit as much as possible inconsistent states by working on the state update order whenever possible, and do additional nullish-check for the edge cases like the rows because we know that avoiding 100% of the inconsistencies will be hard in the long run.