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

[DataGrid] Restore reselect behavior #14410

Merged
merged 3 commits into from
Aug 30, 2024
Merged

[DataGrid] Restore reselect behavior #14410

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe6bab6
fix: restore reselect behavior
romgrk Aug 30, 2024
55657a6
lint
romgrk Aug 30, 2024
8a79138
lint
romgrk Aug 30, 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
10 changes: 9 additions & 1 deletion packages/x-data-grid/src/utils/createSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
import * as React from 'react';
import { createSelector as reselectCreateSelector, Selector, SelectorResultArray } from 'reselect';
import { lruMemoize, createSelectorCreator, Selector, SelectorResultArray } from 'reselect';
import type { GridCoreApi } from '../models/api/gridCoreApi';
import { warnOnce } from '../internals/utils/warning';

type CacheKey = { id: number };

const reselectCreateSelector = createSelectorCreator({
memoize: lruMemoize,
memoizeOptions: {
maxSize: 1,
equalityCheck: Object.is,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NaN values were previously preventing memoization.
reselect@5 added a runtime dev check that caught this, though it wouldn't have happened if they had used Object.is in the first place (which is the only valid equality comparison operator in JS). Also their dev checks are quite expensive, the grid could feel slower in dev mode.

Not the first time I find questionable decisions in reselect.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it makes sense to replace reselect with our own composable selectors implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be nice but I'd place it as low priority, I don't think it would have a big impact on perf or bundle size.

},
});

// TODO v8: Remove this type
export interface OutputSelector<State, Result> {
(apiRef: React.MutableRefObject<{ state: State; instanceId: GridCoreApi['instanceId'] }>): Result;
Expand Down