[DataGrid] Prevent React state update before mount#22374
Conversation
Can't perform a React state update on a component that hasn't mounted yet errorCan't perform a React state update on a component that hasn't mounted yet error
Can't perform a React state update on a component that hasn't mounted yet error
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
There was a problem hiding this comment.
OK, so to better understand these changes, I've compared this implementation to the one before #15648 was merged.
The notable changes are:
updateStatecall on mount, before subscription is active – to pick up store updates missed before mountstoreStatecomparison inupdateState(not sure I understand why – is it really necessary?)- Subscribe in
useLayoutEffectinstead ofuseEffect(useEnhancedEffectvsuseOnMount).
diff --git a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts
index 6680e010ecb..217d27ecf6b 100644
--- a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts
+++ b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts
@@ -1,34 +1,12 @@
+'use client';
import * as React from 'react';
-import { RefObject } from '@mui/x-internals/types';
+import useEnhancedEffect from '@mui/utils/useEnhancedEffect';
+import type { RefObject } from '@mui/x-internals/types';
import { fastObjectShallowCompare } from '@mui/x-internals/fastObjectShallowCompare';
import { warnOnce } from '@mui/x-internals/warning';
import type { GridApiCommon } from '../../models/api/gridApiCommon';
-import type { OutputSelector } from '../../utils/createSelector';
+import type { GridStateCommunity } from '../../models/gridStateCommunity';
import { useLazyRef } from './useLazyRef';
-import { useOnMount } from './useOnMount';
-import type { GridCoreApi } from '../../models/api/gridCoreApi';
-
-function isOutputSelector<Api extends GridApiCommon, Args, T>(
- selector: any,
-): selector is OutputSelector<Api['state'], Args, T> {
- return selector.acceptsApiRef;
-}
-
-type Selector<Api extends GridApiCommon, Args, T> =
- | ((state: Api['state']) => T)
- | OutputSelector<Api['state'], Args, T>;
-
-function applySelector<Api extends GridApiCommon, Args, T>(
- apiRef: RefObject<Api>,
- selector: Selector<Api, Args, T>,
- args: Args,
- instanceId: GridCoreApi['instanceId'],
-) {
- if (isOutputSelector(selector)) {
- return selector(apiRef, args);
- }
- return selector(apiRef.current.state, args, instanceId);
-}
const defaultCompare = Object.is;
export const objectShallowCompare = fastObjectShallowCompare as (a: unknown, b: unknown) => boolean;
@@ -40,7 +18,7 @@ const arrayShallowCompare = (a: any[], b: any[]) => {
return a.length === b.length && a.every((v, i) => v === b[i]);
};
-export const argsEqual = (prev: any, curr: any) => {
+const argsEqual = (prev: any, curr: any) => {
let fn = Object.is;
if (curr instanceof Array) {
fn = arrayShallowCompare;
@@ -50,72 +28,96 @@ export const argsEqual = (prev: any, curr: any) => {
return fn(prev, curr);
};
-const createRefs = () => ({ state: null, equals: null, selector: null, args: null }) as any;
+const createRefs = () =>
+ ({ state: null, equals: null, selector: null, args: undefined, storeState: null }) as any;
+
+const EMPTY = [] as unknown[];
-export const useGridSelector = <Api extends GridApiCommon, Args, T>(
+type Refs<T> = {
+ // `state` stores the selected value returned by this hook. `storeState` stores the
+ // grid store object that produced it, so the mounted-phase catch-up can detect updates
+ // that happened before the subscription was attached without subscribing during render.
+ state: T;
+ storeState: GridStateCommunity | null;
+ equals: <U = T>(a: U, b: U) => boolean;
+ selector: Function;
+ args: any;
+};
+
+export function useGridSelector<Api extends GridApiCommon, T>(
+ apiRef: RefObject<Api>,
+ selector: (apiRef: RefObject<Api>) => T,
+ args?: undefined,
+ equals?: <U = T>(a: U, b: U) => boolean,
+): T;
+export function useGridSelector<Api extends GridApiCommon, T, Args>(
apiRef: RefObject<Api>,
- selector: Selector<Api, Args, T>,
+ selector: (apiRef: RefObject<Api>, a1: Args) => T,
+ args: Args,
+ equals?: <U = T>(a: U, b: U) => boolean,
+): T;
+export function useGridSelector<Api extends GridApiCommon, Args, T>(
+ apiRef: RefObject<Api>,
+ selector: Function,
args: Args = undefined as Args,
equals: <U = T>(a: U, b: U) => boolean = defaultCompare,
-) => {
- if (process.env.NODE_ENV !== 'production') {
- if (!apiRef.current.state) {
- warnOnce([
- 'MUI X: `useGridSelector` has been called before the initialization of the state.',
- 'This hook can only be used inside the context of the grid.',
- ]);
- }
+) {
+ if (!apiRef.current.state) {
+ warnOnce([
+ 'MUI X: `useGridSelector` has been called before the initialization of the state.',
+ 'This hook can only be used inside the context of the grid.',
+ ]);
}
- const refs = useLazyRef<
- {
- state: T;
- equals: typeof equals;
- selector: typeof selector;
- args: typeof args;
- },
- never
- >(createRefs);
+ const refs = useLazyRef<Refs<T>, never>(createRefs);
const didInit = refs.current.selector !== null;
const [state, setState] = React.useState<T>(
// We don't use an initialization function to avoid allocations
- (didInit ? null : applySelector(apiRef, selector, args, apiRef.current.instanceId)) as T,
+ (didInit ? null : selector(apiRef, args)) as T,
);
refs.current.state = state;
+ if (!didInit) {
+ refs.current.storeState = apiRef.current.store.state;
+ }
refs.current.equals = equals;
refs.current.selector = selector;
const prevArgs = refs.current.args;
refs.current.args = args;
if (didInit && !argsEqual(prevArgs, args)) {
- const newState = applySelector(
- apiRef,
- refs.current.selector,
- refs.current.args,
- apiRef.current.instanceId,
- ) as T;
+ const newState = refs.current.selector(apiRef, refs.current.args) as T;
if (!refs.current.equals(refs.current.state, newState)) {
refs.current.state = newState;
setState(newState);
}
+ refs.current.storeState = apiRef.current.store.state;
}
- useOnMount(() => {
- return apiRef.current.store.subscribe(() => {
- const newState = applySelector(
- apiRef,
- refs.current.selector,
- refs.current.args,
- apiRef.current.instanceId,
- ) as T;
- if (!refs.current.equals(refs.current.state, newState)) {
- refs.current.state = newState;
- setState(newState);
+ const updateState = React.useCallback(
+ () => {
+ const storeState = apiRef.current.store.state;
+
+ if (refs.current.storeState !== storeState) {
+ const newState = refs.current.selector(apiRef, refs.current.args) as T;
+ refs.current.storeState = storeState;
+
+ if (!refs.current.equals(refs.current.state, newState)) {
+ refs.current.state = newState;
+ setState(newState);
+ }
}
- });
- });
+ },
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ EMPTY,
+ );
+
+ useEnhancedEffect(() => {
+ updateState();
+ return apiRef.current.store.subscribe(updateState);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, EMPTY);
return state;
-};
+}There was a problem hiding this comment.
I should have taken the approach of reverting to the state before #15648 and working from there instead of building on top of the changes that were already there. The code could be more aligned.
Things that can be changed for points 2 and 3.
storeState comparison in updateState (not sure I understand why – is it really necessary?)
It is not strictly necessary, and the benefit depends on the usage of the useGridSelector hook.
That check returns before the selector is even called. In most cases, the selector call will be a call to createSelector/createSelectorMemoized, so the extra call is cheap, but the impact can be greater with selectors made in the files calling the hook, like
const pipesClassName = useGridSelector(apiRef, () =>
apiRef.current
.unstable_applyPipeProcessors('cellClassName', [], {
id: rowId,
field,
})
.filter(Boolean)
.join(' '),
);
or some other more complex selector created by users.
I am fine dropping this check for more clean code, as this should not create a huge impact.
Subscribe in useLayoutEffect instead of useEffect (useEnhancedEffect vs useOnMount).
Technically, both can work, and in most cases, there will be no difference.
The only difference is in the case when the store is updated synchronously during the commit phase. If that update is related to something the component using the hook renders, there could be a flicker on mount.
There was a problem hiding this comment.
I asked Claude to do an extensive analysis of the impact of removing the check. Here is the result
- The current code comment (lines 37–39) is misleading — it implies storeState is what enables the catch-up to detect pre-mount updates. It isn't. The catch-up detection is done
by re-reading store.state inside updateState(); storeState is just the "last value I processed" baseline. - What the guard actually does: it makes the mount catch-up (and StrictMode's repeated catch-up) a no-op when the store didn't change between render and commit. It does not skip
selectors for "unaffected subscribers" on real store changes — because Store.setState always creates a new state object, every subscriber's updateState always passes the identity
check on a real change and re-runs its selector. So the notification-time perf benefit ≈ 0. - Is it safe to drop? I had an agent exhaustively scan all 320 useGridSelector call sites. Result: zero match the only pattern where dropping changes behavior (an inline selector
returning a fresh object/array under default Object.is). All 7 inline selectors return primitives; the lone fresh-object one (GridRow.tsx:243) already passes objectShallowCompare.
The verifier's counterexample (GridCell:216) was wrong — it ends in .filter(Boolean).join(' '), returning a string. - Bottom line: dropping the guard is behavior-neutral for today's codebase (you and the reviewer are right), costing only one redundant — and cheap — selector call per subscriber
on mount. Keeping it is cheap forward-protection against a future fresh-reference selector + a genuinely nice "catch-up is a clean no-op" property.
So, I would then keep it in there. The description is updated.
| useSyncExternalStore(unsubscribe, subscribe, emptyGetSnapshot); | ||
|
|
There was a problem hiding this comment.
For context, useGridSelector was the precursor to the Store's useStore method. We did the reactivity switch for perf gains, but at the time useSyncExternalStore wasn't widely available yet so I re-wrote useGridSelector as a shim (and also for legacy reasons), with the intent to moving to a simpler implementation in the future. Ideally, we should move towards using useStore (and uSES indirectly), not away from it.
There was a problem hiding this comment.
Thanks for the details.
In this case, we didn't use useSyncExternalStore as it is intended, since we used the snapshot getter to actually subscribe to the state changes, and we didn't use the callback at all.
This is more of a cleanup than moving away from the uSES.
|
Been some time, so I don't have the context in my head though, but from what I recall:
|
Updated the existing test to run in both modes |
romgrk
left a comment
There was a problem hiding this comment.
This hook is a monster and we should get rid of it in favor of useStore, but the current useSyncExternalStore use is indeed a hack. I would highly prefer that we move towards useStore but I don't see any reason to block this PR.
|
Cherry-pick PRs will be created targeting branches: v8.x |
Fixes #17077
The old approach subscribed during render, so it could catch mount-time store updates immediately, but it could also subscribe to components that never mounted.
That caused the SSR warning.
The new approach subscribes only after commit, so unmounted/suspended renders cannot receive store updates. To avoid stale values,
storeStateis compared in the first effect pass.Besides the test for the related issue, I have added tests for
Avoid <GridRoot /> double-render pass on mount in SPA mode#15648, and another to test batched updates based on this conversation https://github.com/mui/mui-x/pull/15648/changes#r1937393895 (which also mentions thatuseSyncExternalStoreis not strictly necessary, but covers all requirements).All tests are failing on the version before #15648 is merged.
Afterwards, only the SSR test is failing, which is a confirmation that they are not false positives and that they can catch future regressions on any of these points.
Before: https://stackblitz.com/edit/github-wpmjdujl-g9fqin9v?file=package.json
After: https://stackblitz.com/edit/github-wpmjdujl-3b5khgfj?file=package.json
CC @lauri865