[DataGrid] Fix gridPageCountSelector selector

[homerchen19]

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/brave-goldberg-3lve8e?file=/demo.tsx

Current behavior 😯

pageCount in the following code always return 1.

const pageCount = useGridSelector(apiRef, gridPageCountSelector);

console.log(pageCount); // 1

From the screenshot, it is apparent that there are 100 rows in total, but the page count is displaying as 1.

Expected behavior 🤔

Based on the codesandbox example, I believe the pageCount should be calculated using the formula Math.ceil(rowCountState / pageSize), which would result in a value of 20.

Context 🔦

No response

Your environment 🌎

npx @mui/envinfo

  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Order ID or Support key 💳 (optional)

No response

[MBilalShafi]

For paginationMode='server' you can use rootProps.rowCount in your CustomPagination component.

I tried to do it on your codesandbox example. Here's the updated codesandbox: https://codesandbox.io/s/awesome-orla-4jcc7h

[homerchen19]

Thank you for getting back to me so quickly! Unfortunately, I don't think your response addresses the issue I reported. The issue I observed relates to "page count" in stead of "row count".
As you can see in the screenshot taken from your codesandbox, the page count appears to be incorrect. I think the correct value should be 20. Since we expect there to be a total of 100 rows (rowCountState is 100) and the page size is 5, the page count should be 100 / 5 = 20, right?

By the way, I encountered this issue when I was trying to migrate data grid to v6. const pageCount = useGridSelector(apiRef, gridPageCountSelector); works fine in v5.

[MBilalShafi]

@homerchen19 You are right, my bad, It had to be rowCount / pageSize, updated the codesandbox.

Actually the confusion comes between TablePagination (default being used in Grid) and Pagination (used in the provided codesandbox).

There is a small interface difference between the two, i.e TablePagination expects rowCount in count prop and the Pagination accepts pageCount.

Compatability with v5:
In v5, we managed rowCount state internally which was being computed like props.rowCount ?? visibleTopLevelRowCount. (or this state was simply reflecting the value passed as a prop or another selector value)

In v6, we removed this state as the value could easily be computed on runtime and doesn't make sense to be stored in a dedicated state. Benefits: Simplify the state and avoid unnecessary complexity.

You can compute the value like being done in the default GridPagination component and pass it to the respective pagination component you are using:

mui-x/packages/grid/x-data-grid/src/components/GridPagination.tsx

Lines 36 to 39 in 6fe709b

	const rowCount = React.useMemo(
	() => rootProps.rowCount ?? visibleTopLevelRowCount ?? 0,
	[rootProps.rowCount, visibleTopLevelRowCount],
	);

So essentially, this is an intended behavior and not a bug.

However, I think removing gridPageCountSelector, since it is not reflecting the correct value in case props.rowCount is passed, is a potential breaking change that we missed during the v6 breaking changes. We could also have documented this in the migration guide, something like:

The gridPageCountSelector was removed. Use specific selectors or props instead:

-const pageCount = useGridSelector(apiRef, gridPageCountSelector);
+const pageSize= useGridSelector(apiRef, gridPageSizeSelector);
+const visibleTopLevelRowCount = useGridSelector(apiRef, gridFilteredTopLevelRowCountSelector);
+const pageCount = getPageCount(rootProps.rowCount ?? visibleTopLevelRowCount , pageSize);

The potential options that we have:

1. Do the breaking change now, provide a codemod to patch it and announce in a release for the users to apply the changes or apply codemod. (Rationale: The selector doesn't reflect the correct value with server-side pagination use cases i.e with props.rowCount)
2. Deprecate the selector, plan it for v7 breaking changes, and recommend applying the above-mentioned diff in JS Doc comment as well as changelog/release notes. (Rationale: We are already stable and some users possibly already migrated)
3. Replace the usages in docs with the diff mentioned above and plan the breaking change for v7. (least possible option)

To me, option 2 seems more like it. What do you think @mui/xgrid team 🤔

[samanthajuniper]

Thank you for reporting this issue, as I am experiencing the same when migrating to v6.

Is there an ETA for when the fix will be merged?

[cherniavskii]

@MBilalShafi
My main concern here is that it would be hard to discover how to properly calculate pageCount.
The snippet required to do this is quite complex:

const apiRef = useGridApiContext();
const pageSize = useGridSelector(apiRef, gridPageSizeSelector);
const visibleTopLevelRowCount = useGridSelector(
  apiRef,
  gridFilteredTopLevelRowCountSelector
);
const rootProps = useGridRootProps();
const pageCount = getPageCount(
  rootProps.rowCount ?? visibleTopLevelRowCount,
  pageSize
);

Would it make sense to pass already calculated rowCount and pageCount as props to the pagination slot component?

[cherniavskii]

@samanthajuniper You can apply the suggestion from #8450 (comment) as a workaround. Here's a working demo: https://codesandbox.io/s/sharp-volhard-zhj42z

[Dustin-Digitar]

Can we at least get this mentioned in the migration docs, please? It has been three months since this was pointed out. It's a pretty serious bug, as anyone who migrates to v6 won't be able to use custom pagination unless they search the github issues and find this thread.

[MBilalShafi]

@Dustin-Digitar apologies for the delay, we'll try to prioritize this asap

[romgrk]

I think we should keep the selector and make it work properly. I think the real issue is that our selectors don't have access to everything they need, and that to solve it we should store more stuff where it needs to be: in the grid state. We're pulling arguments from different react hooks (component-level useState, and various useContext). This means that some of our selectors can't be built cleanly (like gridPageCountSelector), so we have been moving the computations off of the selectors.

We had the same problem with the visible pinned columns in the sticky PR, for context read this. Our selectors should have everything they need, this allows us to build our more complex selectors as a composition over primitive selectors, instead of moving the computations where they don't belong.

[chriszrc]

Wasted an hour playing process of elimination, only to find that the problem had to come from MUI and found this issue. After 6 versions, I really thought that basic pagination would be stable at this point...

[cherniavskii]

@romgrk

to solve it we should store more stuff where it needs to be: in the grid state.

This makes sense, but I'm a bit worried that putting rootProps in the grid state could cause some performance issues as the state will be updated more often making selectors miss the cache more often. What do you think?

[romgrk]

Yeah I haven't made up my mind as to what to put in the state. Putting the props directly is unlikely to be good because most of our users probably don't memoize the props they pass to us properly. The best option would probably be to select which props we put in the state and use something like useEffect(updateGridState, [a, b, c, ...]) where a, b, c, ... are the props we select. It's not as clean but it ensures we don't create a performance issue with the change.

Also I'll note that our expensive complex selectors should be cached on the subparts of the state they depend on, like gridVisiblePinnedColumnDefinitionsSelector does in the link up there. Those subparts don't change even if the root state object changes.

[MBilalShafi]

If we manage to implement #11440 we could possibly skip storing props in the state and provide access to apiRef.current.rootProps to those selectors which need to access some props like rowCount.

Wdyt @romgrk?

[romgrk]

The state has reactivity via .setState, the rest of the API object doesn't. It would also blur the line between the API object and the state object. I would prefer to have them in the state.

[cherniavskii]

@romgrk @MBilalShafi I've created an issue for a general solution for this problem: #11549
I propose to proceed with adding rowCount and pageCount props as an intermediary solution to this issue specifically.
This won't be a breaking change as we won't remove any selectors.
What do you think?

[MBilalShafi]

I propose to proceed with adding rowCount and pageCount props as an intermediary solution to this issue specifically. This won't be a breaking change as we won't remove any selectors.

👍 To me it totally makes sense to start with adding a couple of props we face immediate challenges with, during the process, we can analyze the broader problem addressed in #11549
And yes, I guess it should be a bug fix and not a breaking change in that case.

[romgrk]

I think we should go with the clean general solution as it's not that much more expensive to implement than passing props to pagination, but I don't have strong feelings on this so the other solution is fine with me.

[guicara]

I've lost many hours today with this bug. This should be fixed or at least documented on the component page.

[github-actions]

⚠️ This issue has been closed.
If you have a similar problem, please open a new issue and provide details about your specific problem.
If you can provide additional information related to this topic that could help future readers, please feel free to leave a comment.

How did we do @homerchen19?
Your experience with our support team matters to us. If you have a moment, please share your thoughts through our brief survey.