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.

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

[DataGrid] Fix memory leaks in development #8301

Merged
merged 2 commits into from Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -9,7 +9,6 @@ import type {
GridPrivateOnlyApiCommon,
} from '../../models/api/gridApiCommon';
import { EventManager } from '../../utils/EventManager';
import { unstable_resetCreateSelectorCache } from '../../utils/createSelector';

const isSyntheticEvent = (event: any): event is React.SyntheticEvent => {
return event.isPropagationStopped !== undefined;
Expand Down Expand Up @@ -63,7 +62,7 @@ export function useGridApiInitialization<
if (!publicApiRef.current) {
publicApiRef.current = {
state: {} as Api['state'],
instanceId: globalId,
instanceId: { id: globalId },
} as Api;

globalId += 1;
Expand Down Expand Up @@ -116,7 +115,6 @@ export function useGridApiInitialization<
const api = privateApiRef.current;

return () => {
unstable_resetCreateSelectorCache(api.instanceId);
api.publishEvent('unmount');
};
}, [privateApiRef]);
Expand Down
2 changes: 1 addition & 1 deletion packages/grid/x-data-grid/src/models/api/gridCoreApi.ts
Expand Up @@ -36,7 +36,7 @@ export interface GridCoreApi {
* Unique identifier for each component instance in a page.
* @ignore - do not document.
*/
instanceId: number;
instanceId: { id: number };
}

export interface GridCorePrivateApi<
Expand Down
12 changes: 6 additions & 6 deletions packages/grid/x-data-grid/src/utils/createSelector.test.ts
Expand Up @@ -12,13 +12,13 @@ describe('createSelector', () => {
expect(() => selector(state)).toWarnDev(
'MUI: A selector was called without passing the instance ID, which may impact the performance of the grid.',
);
expect(() => selector(state, 0)).not.toWarnDev();
expect(() => selector(state, { id: 0 })).not.toWarnDev();
});

it('should fallback to the default behavior when no cache key is provided', () => {
const selector = createSelector([], () => []) as OutputSelector<GridStateCommunity, any>;
const state = {} as GridStateCommunity;
const instanceId = 0;
const instanceId = { id: 0 };
expect(selector(state, instanceId)).to.equal(selector(state, instanceId));
});

Expand All @@ -44,21 +44,21 @@ describe('createSelector', () => {
it('should return different selectors for different cache keys', () => {
const selector = createSelector([], () => []) as OutputSelector<GridStateCommunity, any>;
const apiRef1 = {
current: { state: {}, instanceId: 0 },
current: { state: {}, instanceId: { id: 0 } },
} as React.MutableRefObject<GridApiCommunity>;
const apiRef2 = {
current: { state: {}, instanceId: 1 },
current: { state: {}, instanceId: { id: 1 } },
} as React.MutableRefObject<GridApiCommunity>;
expect(selector(apiRef1)).not.to.equal(selector(apiRef2));
});

it('should not clear the cache of one selector when another key is passed', () => {
const selector = createSelector([], () => []) as OutputSelector<GridStateCommunity, any>;
const apiRef1 = {
current: { state: {}, instanceId: 0 },
current: { state: {}, instanceId: { id: 0 } },
} as React.MutableRefObject<GridApiCommunity>;
const apiRef2 = {
current: { state: {}, instanceId: 1 },
current: { state: {}, instanceId: { id: 1 } },
} as React.MutableRefObject<GridApiCommunity>;
const value1 = selector(apiRef1);
selector(apiRef2);
Expand Down
43 changes: 15 additions & 28 deletions packages/grid/x-data-grid/src/utils/createSelector.ts
@@ -1,17 +1,18 @@
import * as React from 'react';
import { createSelector as reselectCreateSelector, Selector, SelectorResultArray } from 'reselect';
import type { GridCoreApi } from '../models/api/gridCoreApi';
import { buildWarning } from './warning';

type CacheKey = number | string;
type CacheKey = { id: number };

interface CacheContainer {
cache: Record<CacheKey, Map<any[], any>> | null;
cache: WeakMap<CacheKey, Map<any[], any>>;
}

export interface OutputSelector<State, Result> {
(apiRef: React.MutableRefObject<{ state: State; instanceId: number }>): Result;
(apiRef: React.MutableRefObject<{ state: State; instanceId: GridCoreApi['instanceId'] }>): Result;
// TODO v6: make instanceId require
(state: State, instanceId?: number): Result;
(state: State, instanceId?: GridCoreApi['instanceId']): Result;
acceptsApiRef: boolean;
}

Expand Down Expand Up @@ -40,48 +41,40 @@ type CreateSelectorFunction = <Selectors extends ReadonlyArray<Selector<any>>, R
...items: SelectorArgs<Selectors, Result>
) => OutputSelector<StateFromSelectorList<Selectors>, Result>;

const cacheContainer: CacheContainer = { cache: null };
const cacheContainer: CacheContainer = { cache: new WeakMap() };

const missingInstanceIdWarning = buildWarning([
'MUI: A selector was called without passing the instance ID, which may impact the performance of the grid.',
'To fix, call it with `apiRef`, e.g. `mySelector(apiRef)`, or pass the instance ID explicitly, e.g `mySelector(state, apiRef.current.instanceId)`.',
]);

export const createSelector: CreateSelectorFunction = (...args: any) => {
if (cacheContainer.cache === null) {
cacheContainer.cache = {};
}

const selector = (...selectorArgs: any[]) => {
const [stateOrApiRef, instanceId] = selectorArgs;
const isApiRef = !!stateOrApiRef.current;
const cacheKey = isApiRef ? stateOrApiRef.current.instanceId : instanceId ?? 'default';
const cacheKey = isApiRef ? stateOrApiRef.current.instanceId : instanceId ?? { id: 'default' };
const state = isApiRef ? stateOrApiRef.current.state : stateOrApiRef;

if (process.env.NODE_ENV !== 'production') {
if (cacheKey === 'default') {
if (cacheKey.id === 'default') {
missingInstanceIdWarning();
}
}

if (cacheContainer.cache === null) {
cacheContainer.cache = {};
}

const { cache } = cacheContainer;

if (cache[cacheKey] && cache[cacheKey].get(args)) {
if (cache.get(cacheKey) && cache.get(cacheKey)?.get(args)) {
// We pass the cache key because the called selector might have as
// dependency another selector created with this `createSelector`.
return cache[cacheKey].get(args)(state, cacheKey);
return cache.get(cacheKey)?.get(args)(state, cacheKey);
}

const newSelector = reselectCreateSelector(...args);

if (!cache[cacheKey]) {
cache[cacheKey] = new Map();
if (!cache.get(cacheKey)) {
cache.set(cacheKey, new Map());
}
cache[cacheKey].set(args, newSelector);
cache.get(cacheKey)?.set(args, newSelector);

return newSelector(state, cacheKey);
};
Expand All @@ -94,12 +87,6 @@ export const createSelector: CreateSelectorFunction = (...args: any) => {
};

// eslint-disable-next-line @typescript-eslint/naming-convention
export const unstable_resetCreateSelectorCache = (cacheKey?: CacheKey) => {
if (typeof cacheKey !== 'undefined') {
if (cacheContainer.cache && cacheContainer.cache[cacheKey]) {
delete cacheContainer.cache[cacheKey];
}
} else {
cacheContainer.cache = null;
}
export const unstable_resetCreateSelectorCache = () => {
cacheContainer.cache = new WeakMap();
};