Skip to content

Conversation

@romgrk
Copy link
Contributor

@romgrk romgrk commented Jun 4, 2025

Import the selectors implementation from base-ui.

@romgrk romgrk added performance scope: data grid Changes related to the data grid. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Jun 4, 2025
export const gridPreferencePanelSelectorWithLabel = createSelector(
gridPreferencePanelStateSelector,
(panel, labelId: string) => {
(panel, labelId: string | undefined) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The typings from the base-ui selector functions are more precise, they can catch more issues.

@mui-bot
Copy link

mui-bot commented Jun 4, 2025

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

Bundle size report

Total Size Change:${\tiny{\color{green}▼}}$-1.69KB(-0.01%) - Total Gzip Change:${\tiny{\color{red}▲}}$+628B(+0.02%)
Files: 120 total (0 added, 0 removed, 6 changed)

@mui/x-data-grid-proparsed:${\tiny{\color{green}▼}}$-612B(-0.13%) gzip:${\tiny{\color{red}▲}}$+112B(+0.09%)
@mui/x-data-grid-premium/DataGridPremiumparsed:${\tiny{\color{green}▼}}$-256B(-0.05%) gzip:${\tiny{\color{red}▲}}$+101B(+0.07%)
@mui/x-data-grid/DataGridparsed:${\tiny{\color{green}▼}}$-202B(-0.06%) gzip:${\tiny{\color{red}▲}}$+124B(+0.12%)
@mui/x-data-gridparsed:${\tiny{\color{green}▼}}$-197B(-0.05%) gzip:${\tiny{\color{red}▲}}$+102B(+0.09%)

Show 2 more bundle changes

@mui/x-data-grid-pro/DataGridProparsed:${\tiny{\color{green}▼}}$-210B(-0.05%) gzip:${\tiny{\color{red}▲}}$+92B(+0.07%)
@mui/x-data-grid-premiumparsed:${\tiny{\color{green}▼}}$-208B(-0.04%) gzip:${\tiny{\color{red}▲}}$+97B(+0.06%)

Details of bundle changes

Generated by 🚫 dangerJS against da80dde

Comment on lines -64 to -67
(apiRef: RefObject<GridApiCommunity>) =>
apiRef.current.state.virtualization.renderContext.firstColumnIndex,
(apiRef: RefObject<GridApiCommunity>) =>
apiRef.current.state.virtualization.renderContext.lastColumnIndex,
Copy link
Contributor Author

@romgrk romgrk Jun 4, 2025

Choose a reason for hiding this comment

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

The create selector functions only accept state selectors, not apiRef selectors, but these aren't public so I think this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

This is a consequence of removing weakMapMemoize and using unwrapIfNeeded, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, every selector passed into createSelector(Memoized) now needs to handle that the first param is apiRefOrState. Only these two inline selectors weren't doing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this in favor of the unwrapIfNeeded logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base-ui selectors support multiple arguments (very efficiently), but we can't because our useGridSelector(api, selector, args?, equals?) wrapper has the equals argument blocking more arguments. We should rework that API at v9, we do need multiple arguments at some places and we use inefficient param objects instead. We could add useGridSelectorV9, and an alternative useGridSelectorWithEquals(api, selector, equals, ...args) for the limited cases where we need equals.

Copy link
Member

Choose a reason for hiding this comment

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

Added to #17846

Copy link
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.

I didn't quite get where the performance-related changes are. Could you clarify this?

Copy link
Member

Choose a reason for hiding this comment

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

Added to #17846

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we use it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oliviertassinari oliviertassinari changed the title [datagrid] Performance: selectors [data grid] Performance: selectors Jun 5, 2025
@romgrk
Copy link
Contributor Author

romgrk commented Jun 5, 2025

I didn't quite get where the performance-related changes are. Could you clarify this?

The old implementation calls reselect's createSelector every time the selector args change.

@romgrk
Copy link
Contributor Author

romgrk commented Jun 5, 2025

I didn't quite get where the performance-related changes are. Could you clarify this?

Also, the previous implementation had two map.get(...) calls for each selector call, whereas this one has a single one. The cache has been inlined inside createSelectorMemoized, so each selector instance has its own cache instead of having to reach for a global one.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 6, 2025
@github-actions
Copy link

github-actions bot commented Jun 6, 2025

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 6, 2025
@romgrk romgrk force-pushed the refactor-selectors branch from f8a512a to 8d62906 Compare June 6, 2025 23:40
@romgrk romgrk force-pushed the refactor-selectors branch from 8d62906 to 8e6196b Compare June 7, 2025 00:11
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 9, 2025
@github-actions
Copy link

github-actions bot commented Jun 9, 2025

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 9, 2025
@romgrk romgrk merged commit 5be7fc1 into mui:master Jun 9, 2025
22 checks passed
@romgrk romgrk deleted the refactor-selectors branch June 9, 2025 20:16
@httpete-broadcom
Copy link

httpete-broadcom commented Jun 17, 2025

I found major changes in the way the grid selectors worked from V7 to V8. Specifically:

import {
  gridPageCountSelector,
  gridPageSelector,
  gridPageSizeSelector,
  useGridApiContext,
} from '@mui/x-data-grid-pro';

These selectors fired in strange ways (multiple firing, resetting the page to 0) when not using the new datasource. I had to work around them to get this upgrade in. In V7 I could switch from the out of the box pagination, or my custom one, based on the state returned by those selectors with zero issue. In V8, even the out of the box one didnt work (with server side pagination, sorting and filtering).

I just tried 8.5.2, and the pagination doesn't work at all with a purely server side grid , and still fails.

@romgrk
Copy link
Contributor Author

romgrk commented Jun 17, 2025

Is it related to the changes in this PR specifically? Can you open a new issue with a reproducible example?

@httpete-broadcom
Copy link

It is not related to this change - I hoped that it was fixed by it. I will work on a case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance scope: data grid Changes related to the data grid. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants