[data grid] fix aggregation with sorting #19892
Conversation
|
Deploy preview: https://deploy-preview-19892--material-ui-x.netlify.app/ Bundle size report
|
|
There's one more issue that should be fixed. In case of row grouping (probably tree data as well) with sorting, there's a flash of unsorted content on first mount. |
|
@romgrk, I noticed that Not sure if I broke something by adding the equality check now, but... |
| const visibleAggregatedFields = visibleColumns | ||
| .slice(renderContext.firstColumnIndex, renderContext.lastColumnIndex + 1) | ||
| .filter((field) => aggregatedFields.includes(field)); | ||
| const visibleAggregatedFieldsWithSort = visibleAggregatedFields.concat( |
There was a problem hiding this comment.
OK, so IIUC instead of calling applyAggregation('sort') on sort model change, sorted columns are now included in the first chunk, correct?
There was a problem hiding this comment.
It's two different things.
applyAggregation('sort') needs to run only when there are sort dependent aggregations (rank, median, first/last, etc.).
But Sort columns were included in the initial chunk due to what I wrote in the next comment. We want them to be included in the first chunk (=these columns affect "viewport" due to potentially changing the row order, regardless of whether they are in the renderContext or not) so that we don't end up with a visual flicker.
There was a problem hiding this comment.
Can you explain why the implementation changed?
There was a problem hiding this comment.
Yes, so, when I went with the previous implementation, I was under the assumption that filtering, etc. doesn't depend on aggregation, hence, we want to guarantee maximum level of deduplication with the queueMicrotask approach. But the case is actually that sorting under certain circumstances (=when sorting an aggregated row with grouping) can depend on aggregation, hence, we cannot defer without incurring visual flicker on mount.
Check this reproduction:
https://stackblitz.com/edit/xdrrihtc?file=src%2FDemo.tsx
| if (!argsEqual(previousState, nextState)) { | ||
| instance.effect(previousState, nextState); | ||
| } |
There was a problem hiding this comment.
I think we can try this optimization, the store is still an evolving API. I would keep it simple though, just Object.is. Do you have any objection to that?
There was a problem hiding this comment.
Object.is is fine. I added the other check initially because I had a thought about replacing multiple events with a single Effect that selects multiple parts of the state. But that was misguided on my part anyways, because the effects are synchronously run on setting state, hence that wouldn't help deduplicate events in any case.
|
@romgrk, another thing I noted on the virtualizer decoupling is that there's now more render passes required to reconcile the initial state for rows to be rendered than before (5 passes in total with static data / columns). In v8.9.1 rows were rendered on the 3rd render pass already vs. 5th. (<~15ms in vs 60ms+ now) |
|
Also, there's no way this can run properly:
Previously inputs were accessed directly from the store (=had access to update values outside of rendering). |
|
Also, the virtualizer used to be isolated from the rest of the Datagrid feature hooks, which was great for performance, since a virtualizer can update quite frequently. Now, by including it in the root of the grid in This is bad imho. And shouldn't have been released under a minor version. |
|
Added an experimental refactor that takes mount performance back to the old baselines if not slightly better (<15ms vs. 60m = 4x better; in 3 render passes vs. 5 currently). The structure is closer to how it used to be as well where Virtualizer doesn't affect the whole grid's feature hook tree with its state updates. No need for hacky commiting setters into a store that consumer that very store (=source for memory leaks), or state setting whenever a ref changes. |
5 participants
Fixes #19370
Alternative to #19891
Avoids cancelling the aggregation process early, as well as includes sortColumns (if aggregated) in the initial chunk / viewport to speed up things visually.