Skip to content

[DataGrid] Restore reselect behavior#14410

Merged
romgrk merged 3 commits intomui:masterfrom
romgrk:fix-reselect-change
Aug 30, 2024
Merged

[DataGrid] Restore reselect behavior#14410
romgrk merged 3 commits intomui:masterfrom
romgrk:fix-reselect-change

Conversation

@romgrk
Copy link
Copy Markdown
Contributor

@romgrk romgrk commented Aug 30, 2024

Follow-up of #11367 (comment)

@romgrk romgrk added the scope: data grid Changes related to the data grid. label Aug 30, 2024
@romgrk romgrk requested a review from cherniavskii August 30, 2024 12:02
@romgrk romgrk mentioned this pull request Aug 30, 2024
1 task
Comment on lines +8 to +13
const reselectCreateSelector = createSelectorCreator({
memoize: lruMemoize,
memoizeOptions: {
maxSize: 1,
}
});
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perf-wise lruMemoize stores its entries an an array, but as we use maxSize: 1 we could write a memoizer that keeps a single value in a variable instead. Don't think it's worth it enough to spend time on it however.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, they have a special case for maxSize 1.

@mui-bot
Copy link
Copy Markdown

mui-bot commented Aug 30, 2024

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

Generated by 🚫 dangerJS against 8a79138

memoize: lruMemoize,
memoizeOptions: {
maxSize: 1,
equalityCheck: Object.is,
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of it!

@romgrk romgrk merged commit 449743e into mui:master Aug 30, 2024
@romgrk romgrk deleted the fix-reselect-change branch August 30, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: data grid Changes related to the data grid.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants