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.

Sign up for GitHub

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

[data grid] Avoid subscribing to renderContext state in grid root for better scroll performance #15986

Merged
merged 15 commits into from
Dec 26, 2024
Merged

[data grid] Avoid subscribing to renderContext state in grid root for better scroll performance #15986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c9386b5
avoid `renderContext` selector hooks in root + better isolation for `…
lauri865 Dec 23, 2024
baea9b1
prettier
lauri865 Dec 23, 2024
1137212
fix row spanning with pagination
lauri865 Dec 23, 2024
703a7e6
add events for sorting and filtering for rowSpanning
lauri865 Dec 23, 2024
8b09405
fix reactivity
lauri865 Dec 23, 2024
e91ffbc
fix prop
lauri865 Dec 23, 2024
6fa1476
missing dep
lauri865 Dec 23, 2024
27b8270
cleanup
lauri865 Dec 23, 2024
1c3ba1d
Merge branch 'master' into fix-rowspanning-performance
lauri865 Dec 23, 2024
cb3cc3d
refactor rowspanning logic
lauri865 Dec 24, 2024
ec53717
remove unused import
lauri865 Dec 24, 2024
ee331f9
fix test
lauri865 Dec 24, 2024
f164dd4
ci
lauri865 Dec 24, 2024
de625c0
Merge branch 'master' into fix-rowspanning-performance
lauri865 Dec 24, 2024
401f0fa
fixes
lauri865 Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/x-data-grid-pro/src/hooks/features/dataSource/useGridDataSource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
GridStrategyGroup,
GridStrategyProcessor,
useGridRegisterStrategyProcessor,
runIf,
} from '@mui/x-data-grid/internals';
import { GridPrivateApiPro } from '../../../models/gridApiPro';
import { DataGridProProcessedProps } from '../../../models/dataGridProProps';
Expand All @@ -26,7 +27,6 @@ import {
DataSourceRowsUpdateStrategy,
NestedDataManager,
RequestStatus,
runIf,
} from './utils';
import { GridDataSourceCache } from '../../../models';
import { GridDataSourceCacheDefault, GridDataSourceCacheDefaultConfig } from './cache';
Expand Down
9 changes: 5 additions & 4 deletions ...es/x-data-grid-pro/src/hooks/features/serverSideLazyLoader/useGridDataSourceLazyLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ import {
GridStrategyGroup,
GridStrategyProcessor,
useGridRegisterStrategyProcessor,
runIf,
} from '@mui/x-data-grid/internals';
import { GridPrivateApiPro } from '../../../models/gridApiPro';
import { DataGridProProcessedProps } from '../../../models/dataGridProProps';
import { findSkeletonRowsSection } from '../lazyLoader/utils';
import { GRID_SKELETON_ROW_ROOT_ID } from '../lazyLoader/useGridLazyLoaderPreProcessors';
import { DataSourceRowsUpdateStrategy, runIf } from '../dataSource/utils';
import { DataSourceRowsUpdateStrategy } from '../dataSource/utils';

enum LoadingTrigger {
VIEWPORT,
Expand Down Expand Up @@ -72,7 +73,6 @@ export const useGridDataSourceLazyLoader = (
const paginationModel = useGridSelector(privateApiRef, gridPaginationModelSelector);
const filteredSortedRowIds = useGridSelector(privateApiRef, gridFilteredSortedRowIdsSelector);
const dimensions = useGridSelector(privateApiRef, gridDimensionsSelector);
const renderContext = useGridSelector(privateApiRef, gridRenderContextSelector);
const renderedRowsIntervalCache = React.useRef(INTERVAL_CACHE_INITIAL_STATE);
const previousLastRowIndex = React.useRef(0);
const loadingTrigger = React.useRef<LoadingTrigger | null>(null);
Expand Down Expand Up @@ -308,6 +308,7 @@ export const useGridDataSourceLazyLoader = (

const handleScrolling: GridEventListener<'scrollPositionChange'> = React.useCallback(
(newScrollPosition) => {
const renderContext = gridRenderContextSelector(privateApiRef);
Copy link
Contributor

@romgrk romgrk Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This kind of change is dangerous in my opinion. If we mix state from useGridSelector with state from the apiRef directly, we are mixing state from different render/update cycles. We should not use this pattern to fix performance issues.

If we need to fix re-render issues, we should specialize selectors. For example, the selector could be targetted for renderContext.lastRowIndex specifically instead of the whole renderContext object, that would prevent re-renders while avoiding bugs.

Copy link
Contributor Author

@lauri865 lauri865 Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There's another PR I opened that takes care of the other useGridSelectors as well, which to me is the correct solution here. For reasons I described in the other PR. #16001

Plus using useGridSelectors in non-rendering hooks makes you write state syncing logic declaratively, which leads to useEffects calling useCallbacks whenever useCallback deps change and other spaghetti that's difficult to debug/trace.

Copy link
Contributor

Choose a reason for hiding this comment

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

which to me is the correct solution here

I'm open to removing code if we can prove that it won't introduce bugs, but mixing state from different update cycles is pretty important to avoid. For example, if part of the state comes from before a filter has been applied, and another part of the state comes from the latest apiRef.current.state after a filter has been applied, we'd be using state slices that aren't coherent with each other.

Using apiRef directly consistently would avoid the issue, though I'd be curious to see how reactive bindings are working.

I'm still catching up with notifications I missed during the holidays but I'll review #16001 in the next days.

Copy link
Contributor Author

@lauri865 lauri865 Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

but mixing state from different update cycles is pretty important to avoid

Completely agree, but you're more likely to do that with by accident with using useGridSelector on root level than not (=mixing stale state from rendering phase with up-to-date state from events/apiRef methods that themselves can hide a state selector in their implementation). Direct state selectors are more commonplace in the code base as it stands today – either directly or indirectly through apiRef methods. Using useGridSelector-s on root-level seems to be a more recent trend in the code base.

Compare the before and after of useGridRowSpanning hook. In which implementation can you answer the question more clearly – under what conditions is the row spanning state reset?

I'm not trying to throw shade to anyone's code in particular. This is not an isolated example, and I could have easily written the same thing myself if I just started out from using useGridSelector instead hooking into the existing events. It's inevitable if you mix two paradigms, you end up writing duct tape to keep them in sync. That duct tape in React is useEffect, which becomes an implicit event that is run under certain conditions. Under which exactly can become quite difficult to understand the more direct and indirect dependencies we end up having, and the less isolated the code is to a particular component.

If you take a step back – no hook in the useDataGridComponent tree is directly responsible for rendering anything – they are not an explicit part of a rendering cycle of any particular component. However, the use of useGridSelector forces those into a declarative paradigm from the outset, while actually serving an imperative function in the context of the data grid.

Just to be clear, even if the motivation around this PR was to take care of a performance regression, the rationale around the other PR (#16001) is not performance per se, it's not me pushing my idea(s/ologies), it's not removing code. But it is to suggest a convention that enables writing more isolated features and easier to follow logic within those features – a foundation that would have prevented this regression in the first place. And it's a pretty simple one.

useGridSelector

  • Perfect to use in component rendering life-cycle (e.g GridRow, GridCell, GridBody, etc.)
  • Should be avoided in root-level feature hooks / when hooking into the global state machine. The only true reason to use useGridSelector on root level is as a direct dependency of a useEffect, however that's seldom done and necessary, and usually a sign of a missing explicit event as useEffect itself will become that event implicitly, but may be invoked much more frequently than necessary.

FWIW, it doesn't seem to be a new convention either from my understanding of the code base. The older versions of the code base either explicitly or implicitly followed this very convention, at least to a much higher degree. Breaking seems to be a more recent trend.

Open to be proven wrong as always – show me an example where useGridSelector in the affected code is provably necessary. I will do my best to showcase why that may actually not be the case, and come up with an alternative on my end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your points make a lot of sense. The reactivity architecture via useGridSelector() has mostly been my effort, following the point #1 of this discussion, and I initially established the guideline of "always use useGridSelector" as a way to provide a simple & clear rule for the rest of the team to follow. This in particular allows to easily refactor code between components & root-level hooks, which I've been doing a lot for the somewhat recent performance improvements.

It's inevitable if you mix two paradigms, you end up writing duct tape to keep them in sync. That duct tape in React is useEffect, which becomes an implicit event that is run under certain conditions. Under which exactly can become quite difficult to understand the more direct and indirect dependencies we end up having, and the less isolated the code is to a particular component.

Yes that's a major source of bugs and it's been on my radar for some time. Our state management goes through various cycles of updates/renders before settling on a coherent state because we use the async useEffect to sync up parts of the state, whereas we should be using something that can synchronously settle on a valid state. For example, if the rows prop is updated, there are various rounds of (async) updates before the filtered, sorted, and grouped rows slices of state are updated to match the new rows prop.

I'm not sure yet what's the best way to solve the issue. Going all in into selectors could be a solution, because selectors can lazily sync with the latest state value, though they keep their state in a memoized memory cell that doesn't live on the state object so that's less debuggable. Alternatively, we could create some event-based system that updates the state synchronously when another part of the state changes, this would not be lazy but it would be more debuggable.

useGridSelector: Perfect to use in component rendering life-cycle / Should be avoided in root-level feature hooks

I'm ok with that convention but it places a burden on the maintainers to be strict about observing those invariants, whereas creating specialized selectors would provide correctness without having to think about the context before using state data. Let me ping the team to see what's the consensus.

Copy link
Member

@MBilalShafi MBilalShafi Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Really nice discussion points. A couple of cents from my side.

If we mix state from useGridSelector with state from the apiRef directly, we are mixing state from different render/update cycles.

Yes, this should be avoided as much as possible, I supported the change in #15986 specific to the renderContext as this particular state update is a bit unusual, I suspected the performance lag in Safari is due to wrapping of the state update in ReactDOM.flushSync, extracting the value in the event listener seemed like a proper reactive handle for this case. Also supported by the fact that yielded a better scroll performance. The testing I did didn't exhibit any regressions so I moved forward to merge, we could revert if we find one.

An alternate approach for this event (handleScrolling) could be to either extract the other state (dimensions) in the function too or use the argument (params.renderContext) passed to the event handler. 1 would avoid mixing of useGridSelector and apiRef slices, while 2 would remove state out-of-sync without extracting the selector slice. (But only for this instance, we'd need to do similar things for other (useGridRowSpanning)).

useGridSelector should be avoided in root-level feature hooks

If we manage to make that happen cleanly, it'll definitely make it harder for inactive features to impact the performance or unnecessarily re-render/re-compute some parts of the application, improving the overall performance, however, I'd be careful to avoid breaking an expected reactivity with this change.

All our root-level hooks are grouped, so it wouldn't be too hard to do:

I assume there are some use cases where it's inevitable to avoid useGridSelector in root hooks, like when they are used as useEffect dependencies. We'd need to provide an escape hatch to make that happen.

Copy link
Contributor Author

@lauri865 lauri865 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So in the feature hooks, we use plain state selectors in event listeners, and if there's no event, do we use useGridSelector + useEffect?

I think the first question to ask if the event is missing is whether a new event should be added. If a state change triggers another piece of state to change as a side-effect, useEffect/useLayoutEffect are always worse options to write that logic in React than an event, as things will now happen in different render passes.

useEffect is an escape hatch from declarative rendering paradigm in react. In theory, the only appropriate use for useEffect in feature hooks (in relation to the internal state management) should be to sync props with the internal state. Of course there are other appropriate cases like subscribing to DOM events, etc.

I didn't see any obvious examples where I thought useGridSelector would be necessary in feature hooks, outside of useGridDimensions and useGridRowsMeta (although that could be fixed with more work as well, but since it's a core hook rather than feature hook, it's less of a concern to me; readability is much worse though with useGridSelector).

I'm curious to see if you have any examples though that would prove the inverse. The only theoretical case for useGridSelector I can think of – an event that mutates internal state needs to access the rendered value of the state (=old state). The only other case I can think of is that a feature hook needs to send the grid to re-render when no component depends on that state – but that smells of side-effects, and likely has a much nicer solution. If such cases would exist, they would likely be broken by react compiler in the future as well. But I haven't found any such cases yet, and this change would have highlighted them as well:
#15666 (comment)

Copy link
Contributor Author

@lauri865 lauri865 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, almost forgot. An alternative escape hatch to useGridSelector/useEffect could be useGridSelectorChange. Instead of creating/updating state, it would call a callback if the resulting selector value changes.

But I haven't seen a place where I would use it yet, except for useGridDimensions / useGridRowsMeta.

Copy link
Member

Choose a reason for hiding this comment

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

The only theoretical case for useGridSelector I can think of – an event that mutates internal state needs to access the rendered value of the state (=old state)

In this case, the old state = the current state (before the new one is applied), so a regular selector should work here. Does this make sense?

Copy link
Contributor Author

@lauri865 lauri865 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The only theoretical case for useGridSelector I can think of – an event that mutates internal state needs to access the rendered value of the state (=old state)

In this case, the old state = the current state (before the new one is applied), so a regular selector should work here. Does this make sense?

I meant a case, e.g.:

  • onKeyDown DOM event fires
  • focusCell state is changed (but updated state is not rendered until the next keyframe)
  • onCellKeyDown internal event is fired
  • For whatever reason, the onCellKeyDown consumer in a feature hook (=different hook than the one that manages focus state) needs to know the currently rendered focusCell

It's all purely theoretical though – I was just stretching my imagination to come up with a case where useGridSelector would be a necessary choice over straight selectors in callbacks (as I proposed), and hence tried to poke holes in my other PR. For avoidance of doubt, I haven't seen any practical example in the code where this is an actual use case.

if (
loadingTrigger.current !== LoadingTrigger.SCROLL_END ||
previousLastRowIndex.current >= renderContext.lastRowIndex
Expand Down Expand Up @@ -342,7 +343,6 @@ export const useGridDataSourceLazyLoader = (
filterModel,
dimensions,
paginationModel.pageSize,
renderContext.lastRowIndex,
adjustRowParams,
],
);
Expand Down Expand Up @@ -419,6 +419,7 @@ export const useGridDataSourceLazyLoader = (
(newSortModel) => {
rowsStale.current = true;
previousLastRowIndex.current = 0;
const renderContext = gridRenderContextSelector(privateApiRef);
const rangeParams =
loadingTrigger.current === LoadingTrigger.VIEWPORT
? {
Expand All @@ -442,7 +443,7 @@ export const useGridDataSourceLazyLoader = (
adjustRowParams(getRowsParams),
);
},
[privateApiRef, filterModel, paginationModel.pageSize, renderContext, adjustRowParams],
[privateApiRef, filterModel, paginationModel.pageSize, adjustRowParams],
);

const handleGridFilterModelChange = React.useCallback<GridEventListener<'filterModelChange'>>(
Expand Down
200 changes: 101 additions & 99 deletions packages/x-data-grid/src/hooks/features/rows/useGridRowSpanning.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,18 @@ import * as React from 'react';
import useLazyRef from '@mui/utils/useLazyRef';
import { GRID_DETAIL_PANEL_TOGGLE_FIELD } from '../../../internals/constants';
import { gridVisibleColumnDefinitionsSelector } from '../columns/gridColumnsSelector';
import { useGridVisibleRows } from '../../utils/useGridVisibleRows';
import { getVisibleRows } from '../../utils/useGridVisibleRows';
import { gridRenderContextSelector } from '../virtualization/gridVirtualizationSelectors';
import { useGridSelector } from '../../utils/useGridSelector';
import { gridRowTreeSelector } from './gridRowsSelector';
import { GridRenderContext } from '../../../models';
import type { GridColDef } from '../../../models/colDef';
import type { GridRowId, GridValidRowModel, GridRowEntry } from '../../../models/gridRows';
import type { DataGridProcessedProps } from '../../../models/props/DataGridProps';
import type { GridPrivateApiCommunity } from '../../../models/api/gridApiCommunity';
import type { GridStateInitializer } from '../../utils/useGridInitializeState';
import {
getUnprocessedRange,
isRowRangeUpdated,
isRowContextInitialized,
getCellValue,
} from './gridRowSpanningUtils';
import { getUnprocessedRange, isRowContextInitialized, getCellValue } from './gridRowSpanningUtils';
import { GRID_CHECKBOX_SELECTION_FIELD } from '../../../colDef/gridCheckboxSelectionColDef';
import { useGridApiEventHandler } from '../../utils/useGridApiEventHandler';
import { runIf } from '../../../utils/utils';

export interface GridRowSpanningState {
spannedCells: Record<GridRowId, Record<GridColDef['field'], number>>;
Expand Down Expand Up @@ -94,9 +90,10 @@ const computeRowSpanningState = (
const backwardsHiddenCells: number[] = [];
if (index === rangeToProcess.firstRowIndex) {
let prevIndex = index - 1;
const prevRowEntry = visibleRows[prevIndex];
let prevRowEntry = visibleRows[prevIndex];
while (
prevIndex >= range.firstRowIndex &&
prevRowEntry &&
getCellValue(prevRowEntry.model, colDef, apiRef) === cellValue
) {
const currentRow = visibleRows[prevIndex + 1];
Expand All @@ -110,6 +107,8 @@ const computeRowSpanningState = (
spannedRowId = prevRowEntry.id;
spannedRowIndex = prevIndex;
prevIndex -= 1;

prevRowEntry = visibleRows[prevIndex];
}
}

Expand Down Expand Up @@ -165,69 +164,66 @@ const computeRowSpanningState = (
* @requires filterStateInitializer (method) - should be initialized before
*/
export const rowSpanningStateInitializer: GridStateInitializer = (state, props, apiRef) => {
if (props.rowSpanning) {
const rowIds = state.rows!.dataRowIds || [];
const orderedFields = state.columns!.orderedFields || [];
const dataRowIdToModelLookup = state.rows!.dataRowIdToModelLookup;
const columnsLookup = state.columns!.lookup;
const isFilteringPending =
Boolean(state.filter!.filterModel!.items!.length) ||
Boolean(state.filter!.filterModel!.quickFilterValues?.length);

if (
!rowIds.length ||
!orderedFields.length ||
!dataRowIdToModelLookup ||
!columnsLookup ||
isFilteringPending
) {
return {
...state,
rowSpanning: EMPTY_STATE,
};
}
const rangeToProcess = {
firstRowIndex: 0,
lastRowIndex: Math.min(DEFAULT_ROWS_TO_PROCESS, Math.max(rowIds.length, 0)),
if (!props.rowSpanning) {
return {
...state,
rowSpanning: EMPTY_STATE,
};
const rows = rowIds.map((id) => ({
id,
model: dataRowIdToModelLookup[id!],
})) as GridRowEntry<GridValidRowModel>[];
const colDefs = orderedFields.map((field) => columnsLookup[field!]) as GridColDef[];
const { spannedCells, hiddenCells, hiddenCellOriginMap } = computeRowSpanningState(
apiRef,
colDefs,
rows,
rangeToProcess,
rangeToProcess,
true,
EMPTY_RANGE,
);
}

const rowIds = state.rows!.dataRowIds || [];
const orderedFields = state.columns!.orderedFields || [];
const dataRowIdToModelLookup = state.rows!.dataRowIdToModelLookup;
const columnsLookup = state.columns!.lookup;
const isFilteringPending =
Boolean(state.filter!.filterModel!.items!.length) ||
Boolean(state.filter!.filterModel!.quickFilterValues?.length);

if (
!rowIds.length ||
!orderedFields.length ||
!dataRowIdToModelLookup ||
!columnsLookup ||
isFilteringPending
) {
return {
...state,
rowSpanning: {
spannedCells,
hiddenCells,
hiddenCellOriginMap,
},
rowSpanning: EMPTY_STATE,
};
}
const rangeToProcess = {
firstRowIndex: 0,
lastRowIndex: Math.min(DEFAULT_ROWS_TO_PROCESS, Math.max(rowIds.length, 0)),
};
const rows = rowIds.map((id) => ({
id,
model: dataRowIdToModelLookup[id!],
})) as GridRowEntry<GridValidRowModel>[];
const colDefs = orderedFields.map((field) => columnsLookup[field!]) as GridColDef[];
const { spannedCells, hiddenCells, hiddenCellOriginMap } = computeRowSpanningState(
apiRef,
colDefs,
rows,
rangeToProcess,
rangeToProcess,
true,
EMPTY_RANGE,
);

return {
...state,
rowSpanning: EMPTY_STATE,
rowSpanning: {
spannedCells,
hiddenCells,
hiddenCellOriginMap,
},
};
};

export const useGridRowSpanning = (
apiRef: React.MutableRefObject<GridPrivateApiCommunity>,
props: Pick<DataGridProcessedProps, 'rowSpanning' | 'pagination' | 'paginationMode'>,
): void => {
const { range, rows: visibleRows } = useGridVisibleRows(apiRef, props);
const renderContext = useGridSelector(apiRef, gridRenderContextSelector);
const colDefs = useGridSelector(apiRef, gridVisibleColumnDefinitionsSelector);
const tree = useGridSelector(apiRef, gridRowTreeSelector);
const processedRange = useLazyRef<RowRange, void>(() => {
return Object.keys(apiRef.current.state.rowSpanning.spannedCells).length > 0
? {
Expand All @@ -239,23 +235,13 @@ export const useGridRowSpanning = (
}
: EMPTY_RANGE;
});
const lastRange = React.useRef<RowRange>(EMPTY_RANGE);

const updateRowSpanningState = React.useCallback(
// A reset needs to occur when:
// - The `unstable_rowSpanning` prop is updated (feature flag)
// - The filtering is applied
// - The sorting is applied
// - The `paginationModel` is updated
// - The rows are updated
(resetState: boolean = true) => {
if (!props.rowSpanning) {
if (apiRef.current.state.rowSpanning !== EMPTY_STATE) {
apiRef.current.setState((state) => ({ ...state, rowSpanning: EMPTY_STATE }));
}
return;
}

(renderContext: GridRenderContext, resetState: boolean = false) => {
const { range, rows: visibleRows } = getVisibleRows(apiRef, {
pagination: props.pagination,
paginationMode: props.paginationMode,
});
if (range === null || !isRowContextInitialized(renderContext)) {
return;
}
Expand All @@ -276,6 +262,7 @@ export const useGridRowSpanning = (
return;
}

const colDefs = gridVisibleColumnDefinitionsSelector(apiRef);
const {
spannedCells,
hiddenCells,
Expand Down Expand Up @@ -306,8 +293,9 @@ export const useGridRowSpanning = (
resetState ||
newSpannedCellsCount !== currentSpannedCellsCount ||
newHiddenCellsCount !== currentHiddenCellsCount;
const hasNoSpannedCells = newSpannedCellsCount === 0 && currentSpannedCellsCount === 0;

if (!shouldUpdateState) {
if (!shouldUpdateState || hasNoSpannedCells) {
return;
}

Expand All @@ -322,35 +310,49 @@ export const useGridRowSpanning = (
};
});
},
[apiRef, props.rowSpanning, range, renderContext, visibleRows, colDefs, processedRange],
[apiRef, processedRange, props.pagination, props.paginationMode],
);

const prevRenderContext = React.useRef(renderContext);
const isFirstRender = React.useRef(true);
const shouldResetState = React.useRef(false);
const previousTree = React.useRef(tree);
React.useEffect(() => {
const firstRender = isFirstRender.current;
if (isFirstRender.current) {
isFirstRender.current = false;
}
if (tree !== previousTree.current) {
previousTree.current = tree;
updateRowSpanningState(true);
// Reset events trigger a full re-computation of the row spanning state:
// - The `unstable_rowSpanning` prop is updated (feature flag)
// - The filtering is applied
// - The sorting is applied
// - The `paginationModel` is updated
// - The rows are updated
const resetRowSpanningState = React.useCallback(() => {
const renderContext = gridRenderContextSelector(apiRef);
if (!isRowContextInitialized(renderContext)) {
return;
}
if (range && lastRange.current && isRowRangeUpdated(range, lastRange.current)) {
lastRange.current = range;
shouldResetState.current = true;
}
if (!firstRender && prevRenderContext.current !== renderContext) {
if (isRowRangeUpdated(prevRenderContext.current, renderContext)) {
updateRowSpanningState(shouldResetState.current);
shouldResetState.current = false;
updateRowSpanningState(renderContext, true);
}, [apiRef, updateRowSpanningState]);

useGridApiEventHandler(
apiRef,
'renderedRowsIntervalChange',
runIf(props.rowSpanning, updateRowSpanningState),
);

useGridApiEventHandler(apiRef, 'sortedRowsSet', runIf(props.rowSpanning, resetRowSpanningState));
useGridApiEventHandler(
apiRef,
'paginationModelChange',
runIf(props.rowSpanning, resetRowSpanningState),
);
useGridApiEventHandler(
apiRef,
'filteredRowsSet',
runIf(props.rowSpanning, resetRowSpanningState),
);
useGridApiEventHandler(apiRef, 'columnsChange', runIf(props.rowSpanning, resetRowSpanningState));

React.useEffect(() => {
if (!props.rowSpanning) {
if (apiRef.current.state.rowSpanning !== EMPTY_STATE) {
apiRef.current.setState((state) => ({ ...state, rowSpanning: EMPTY_STATE }));
}
prevRenderContext.current = renderContext;
return;
} else if (apiRef.current.state.rowSpanning === EMPTY_STATE) {
resetRowSpanningState();
}
updateRowSpanningState();
}, [updateRowSpanningState, renderContext, range, lastRange, tree]);
}, [apiRef, resetRowSpanningState, props.rowSpanning]);
};
7 changes: 5 additions & 2 deletions packages/x-data-grid/src/tests/rowSpanning.DataGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ describe('<DataGrid /> - Row spanning', () => {

describe('rows update', () => {
it('should update the row spanning state when the rows are updated', () => {
const rowSpanValueGetter = spy();
const rowSpanValueGetter = spy((value) => value);
let rowSpanningStateUpdates = 0;
let spannedCells = {};
render(
Expand All @@ -271,7 +271,10 @@ describe('<DataGrid /> - Row spanning', () => {
expect(rowSpanningStateUpdates).to.equal(1);

act(() => {
apiRef.current.setRows([{ id: 1, code: 'A101' }]);
apiRef.current.setRows([
{ id: 1, code: 'A101' },
{ id: 2, code: 'A101' },
]);
});

// Second update on row update
Expand Down
6 changes: 6 additions & 0 deletions packages/x-data-grid/src/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,9 @@ export function deepClone(obj: Record<string, any>) {
* that hint disables checks on all values instead of just one.
*/
export function eslintUseValue(_: any) {}

export const runIf = (condition: boolean, fn: Function) => (params: unknown) => {
if (condition) {
fn(params);
}
};
Loading