[Data Grid] Avoid <GridRoot /> double-render pass on mount in SPA mode

[lauri865]

• I have followed (at least) the PR section of the contributing guide.

Currently, GridRoot double-renders in all cases on mount to prevent SSR. In SPA-mode this is irrelevant, and can be avoided with the help of useSyncExternalStore, since there's no server snapshot in SPA-mode, the gird will be mounted directly. As a result, the rootRef becomes immediately available on first mount in SPAs, as one would expect.

Adds a new dependency use-sync-external-store to make it backwards compatible with React 17. Charts and treeview have the same dependency.

[mui-bot]

Deploy preview: https://deploy-preview-15648--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 4b7f544

[lauri865]

Side-benefit is also probably more robust tests if the Datagrid is mounted on the first pass.

[romgrk]

Code LGTM, we can merge once the tests are passing.

[lauri865]

I have no idea how to fix the last failing test. Either it's a symptom of column headers updated more times now or just a bad test. But I lack understanding why it was previously expected that the warning message will be output exactly twice.

Edit: discovered some areas to improve around mounting / state initialization

[lauri865]

Also fixes an annoying flicker coming out of loading state. It's probably the root cause of a layout shift I once reported with autoHeight grid coming out of loading state.

Before:
https://github.com/user-attachments/assets/1a68b2de-f2fd-4ef4-a93d-215d044e35b7

After:
https://github.com/user-attachments/assets/cfe55012-092a-429d-a79d-75e243124d10

[lauri865]

Tests should be passing now. 4 was the right amount of warnings for this approach, 1 extra pass (+1 warning for header/row each = 2+2 = 4) that doesn't even flush to the dom, so I think we're good there.

Curiously, after this optimisation:

mui-x/packages/x-data-grid/src/hooks/features/columns/useGridColumns.tsx

Lines 400 to 412 in 5ca918f

	const hasFlexColumns = gridVisibleColumnDefinitionsSelector(apiRef).some(
	(col) => col.flex && col.flex > 0,
	);
	if (!hasFlexColumns) {
	return;
	}
	setGridColumnsState(
	hydrateColumnsWidth(
	gridColumnsStateSelector(apiRef.current.state),
	apiRef.current.getRootDimensions(),
	),
	);

This test started failing:

mui-x/packages/x-data-grid-premium/src/tests/aggregation.DataGridPremium.test.tsx

Lines 145 to 150 in 1a079e1

	it('should correctly restore the column when changing from aggregated to non-aggregated', () => {
	const { setProps } = render(<Test aggregationModel={{ id: 'max' }} />);
	expect(getColumnHeaderCell(0, 0).textContent).to.equal('idmax');
	setProps({ aggregationModel: {} });
	expect(getColumnHeaderCell(0, 0).textContent).to.equal('id');
	});

The only reason I can think of is that aggregations didn't explicitly trigger column updates, but somehow were piggybacking on viewportInnerSizeChange event for taking care of it. Adding an explicit updateColumns call fixed it. Is there any other explanation to it?

If that's the case, then I suppose there was a bug whereby columns didn't clear when there was no totals row visible, as viewPortInnerSize would only change if there was a pinned row.

[romgrk]

Why is 1 decimal place a better value?

[lauri865]

Some browsers support subpixel positioning, but I saw some instances in the docs where width was 242.32px, which triggers unnecessary updates. Happy to change it to full pixels if that makes more sense.

[romgrk]

Why is this test modified?

[lauri865]

Seemed like a faulty premise for the test, but honestly in hindsight, I'm not sure why it previously worked to begin with.

Lazy loader only subscribes to renderedRowsIntervalChange:

mui-x/packages/x-data-grid-pro/src/hooks/features/lazyLoader/useGridLazyLoader.ts

Lines 134 to 138 in 1a079e1

	useGridApiEventHandler(
	privateApiRef,
	'renderedRowsIntervalChange',
	handleRenderedRowsIntervalChange,
	);

scrollPositionChange shouldn't change actual scroll position nor trigger renderedRowsIntervalChange, so how can it even trigger a new slice to be loaded?

Maybe you understand it better, but I tried to test it in the browser and everything worked fine with actual scrolling.

[lauri865]

This test gave me so much grief trying to trace in the code base how scrollPositionChange can possibly trigger lazyLoader. And guess what? It doesn't.

Ran it on the master branch and removing the publishEvent line will still make the pass. But for whatever reason, it will always try to load all the rows minus the first 3 that it has – tha args to onFetchRows are (I set the page size to 500):

Object{firstRowToRender: 3, lastRowToRender: 500, sortModel: [], filterModel: Object{items: [], logicOperator: 'and', quickFilterValues: [], quickFilterLogicOperator: 'and'}}

Again, this is on the master branch, for the lack of any confusion.

[lauri865]

The test is still testing something useful that broke, just not what it says on the label currently. I reduced the amount updateDimensions calls, which called updateRenderContext ahead of dimensions state being updated, which prevented lazyLoader from working directly on mount (scroll still worked).

Fixed it now, and improved the tests – split it into two and improved the checks to more than just checking if fetchRows has been called. Hopefully no-one else will have to waste so much time on this anymore.

[romgrk]

Why is this required?

While you're touching this, you could delete .forceUpdate(), it's a legacy function that should be removed.

[lauri865]

After some further debugging, it's not required as such, but there's a bug somehwere:

• Removing aggregation via prop triggers checkAggregationRulesDiff -> triggers hydrateColumns -> updates columns (but aggregation is not cleared for whatever reason) -> triggers viewportInnerSizeChange -> triggers handleGridSizeChange (which often is unnecessary, unless there are flex columns) where aggregation is actually cleared.

So, it seems that hydrateColumns is probably ran before state has resolved.

The right fix then is to replace:

useGridApiOptionHandler(apiRef, 'aggregationModelChange', checkAggregationRulesDiff);

With something along the lines of (unless there's a better internal way to chain this):

const aggregationModel = useGridSelector(apiRef, gridAggregationModelSelector);
useEnhancedEffect(checkAggregationRulesDiff, [checkAggregationRulesDiff, aggregationModel]);

[lauri865]

Root cause is that these two get triggered out of order:

useGridApiOptionHandler(apiRef, 'aggregationModelChange', checkAggregationRulesDiff);

React.useEffect(() => {
    if (props.aggregationModel !== undefined) {
      apiRef.current.setAggregationModel(props.aggregationModel);
    }
  }, [apiRef, props.aggregationModel]);

As soon as the prop changes, checkAggregationRulesDiff is triggered, ahead of setAggregationModel, but columns cannot rehydrate before aggregationModel has been set. And it currently only works, because column update always triggers two state updates to columns (which in most cases is not needed), which triggers a second hydration from aggregation, only at which point the aggregation state has been updated.

[lauri865]

cc @flaviendelangle, any thoughts? 🙏 Seems like you worked on this feature.

[flaviendelangle]

I built the feature 2.5 years ago so my understanding of it might be rusted 😆

As soon as the prop changes, checkAggregationRulesDiff is triggered

That surprises me.
checkAggregationRulesDiff is triggered by the aggregationModelChange event, which is fired in the setState method, which is called by setAggregationModel

So basically:

props.aggregationModel changes => setState is called => state.aggregation.model is updated and aggregationModelChange is fired => checkAggregationRulesDiff is called

Maybe we have a problem during the initialization though.

The problem with firing updated like checkAggregationRulesDiff in effects is that you might end up with inconsistent states and flickering, but I don't know if that's the case for this specific feature.

[lauri865]

There problem seems to be on initialization indeed. Not sure if it has any real-world bearing, but on first prop change, the values are as such:

aggregationRules {}
rulesOnLastColumnHydration {}
aggregationRules {}
areAggregationRulesEqual(rulesOnLastColumnHydration, aggregationRules)

rulesOnLastColumnHydration is empty and thus hydrateColumns is not triggered directly after the prop change, even though columns have rules applied as can be seen from the test result:

1) <DataGridPremium /> - Aggregation
       Setting aggregation model
         prop: aggregationModel
           should correctly restore the column when changing from aggregated to non-aggregated:

      AssertionError: expected 'idmax' to equal 'id'
      + expected - actual

      -idmax
      +id

[lauri865]

This hack also helps the test pass, but not sure what the actual fix here is:

export const aggregationStateInitializer: GridStateInitializer<
  Pick<DataGridPremiumProcessedProps, 'aggregationModel' | 'initialState'>,
  GridPrivateApiPremium
> = (state, props, apiRef) => {
  apiRef.current.caches.aggregation = {
    rulesOnLastColumnHydration: props.aggregationModel !== undefined ? ({ _init: {} } as any) : {},
    rulesOnLastRowHydration: {},
  };

  return {
    ...state,
    aggregation: {
      model: props.aggregationModel ?? props.initialState?.aggregation?.model ?? {},
    },
  };
};

[lauri865]

aha, I think I see the actual fix now

[lauri865]

I think the right place to update apiRef.current.caches.aggregation.rulesOnLastColumnHydration is in the preprocessor after hydrateColumns and not in checkAggregationRulesDiff

[romgrk]

The argos CI test has a few failing cases, some of which would need to be fixed (at least the "no rows" one).

[lauri865]

"no rows" one should be fixed now. Created a selector for pinnedRow heights, which improves updateDimensions flow of updates (imho).

[lauri865]

Ok, so it seems like useGridSelector can under certain circumstances return the stale state:

• Component mounts
• Selector sets initialState
• Something triggers a state change before Component can mount (i.e. before useOnMount/useEffect where store subscription is added)
• Store subscription is added too late, we don't catch the state update, and internal state of the selector remains stale.

I will change useOnMount to useEnhancedEffect with an empty dependency array, unless there are any objections @romgrk? Store.subscribe is a very simple operation, so shouldn't make any difference performance-wise.

[lauri865]

Tests passing 🎉
(this few line change turned out to be quite a can of worms, but all for the better I hope 🙈)

[github-actions]

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