[data-grid] Controlled pagination issue

[cq-ubaid-khan]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Current behavior 😯

I have a controlled pagination with onPageChange and onPageSizeChange functions applied on datagrid and pagination mode sets to "server", But sometimes when changing pageSize the onPageSizeChange is called but onPageChange also gets called automatically.

Expected behavior 🤔

I am expecting that both the onPageChange and onPageSizeChange functions should work independently.

Steps to reproduce 🕹

https://codesandbox.io/s/quirky-tu-i0nox?file=/src/Demo.tsx

Steps:

1. Open console window and observe the output
2. Click pagination next button 3 times and then clear console
3. Now change the page size to 10

You will probably see the output something like this

Output:

Current Page Size: 10
Current Page: 2

Context 🔦

I have ajax calls in both functions and currently same ajax calls are being sent multiple times and page state also gets reverted to old state.

Your environment 🌎

`npx @mui/envinfo`

  Browser: 
    Chrome Version 96.0.4664.110
  System:
    OS: Windows 10 10.0.19043
  Binaries:
    Node: 14.16.1 - C:\Program Files\nodejs\node.EXE
    Yarn: Not Found
    npm: 6.14.12 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: Not Found
    Edge: Spartan (44.19041.1266.0), Chromium (96.0.1054.62)
  npmPackages:
    @emotion/react: ^11.7.1 => 11.7.1
    @emotion/styled: ^11.6.0 => 11.6.0
    @mui/base:  5.0.0-alpha.60
    @mui/icons-material: ^5.2.1 => 5.2.4
    @mui/material: ^5.2.4 => 5.2.4
    @mui/private-theming:  5.2.3
    @mui/styled-engine:  5.2.4
    @mui/system:  5.2.4
    @mui/types:  7.1.0
    @mui/utils:  5.2.3
    @mui/x-data-grid-pro: ^5.2.0 => 5.2.0
    @mui/x-license-pro:  5.2.0
    @types/react: ^16.9.41 => 16.14.21
    react: ^16.13.1 => 16.14.0
    react-dom: ^16.8.3 => 16.14.0
    typescript: ^3.9.6 => 3.9.10

[alexfauquette]

This sounds more like protection than a bug.

In your example rowCount=28. So if you try to get page 3 with size 10, you will be out of the range. Then the DataGrid moves the page to the closest one in the range (page 2)

A solution is to check in onPageSizeChange if the selected page is still in the range, and if not modify it yourself. this will prevent the onPageChange to be triggered. I propose the following from your example

onPageSizeChange={(pageSize) => {
	  console.log("Current Page Size: ", pageSize);
	  setSelectedPageSize(pageSize);
	  
	  const lastPageIndex = Math.max(
	    0,
	    Math.floor((ROW_NUMBER - 1) / pageSize)
	  );
	  if (selectedPage > lastPageIndex) {
	    setSelectedPage(lastPageIndex);
	  }
  }}

You can experiment it in the following code sandbox: https://codesandbox.io/s/happy-brook-cmity?file=/src/Demo.tsx

@mui-org/x I'm wondering if we should display a warning when the page is out of the range. For example, it could be

page ${pageIndex} is out of range for rowCount=${rowCount} and pageSize={$pageSize}.
Corrected by page=${correctedPageIndex}

Another solution would be to add a warning in the doc

[cq-ubaid-khan]

This sounds more like protection than a bug.

In your example rowCount=28. So if you try to get page 3 with size 10, you will be out of the range. Then the DataGrid moves the page to the closest one in the range (page 2)

A solution is to check in onPageSizeChange if the selected page is still in the range, and if not modify it yourself. this will prevent the onPageChange to be triggered. I propose the following from your example

onPageSizeChange={(pageSize) => {
	  console.log("Current Page Size: ", pageSize);
	  setSelectedPageSize(pageSize);
	  
	  const lastPageIndex = Math.max(
	    0,
	    Math.floor((ROW_NUMBER - 1) / pageSize)
	  );
	  if (selectedPage > lastPageIndex) {
	    setSelectedPage(lastPageIndex);
	  }
  }}

You can experiment it in the following code sandbox: https://codesandbox.io/s/happy-brook-cmity?file=/src/Demo.tsx

@mui-org/x I'm wondering if we should display a warning when the page is out of the range. For example, it could be

page ${pageIndex} is out of range for rowCount=${rowCount} and pageSize={$pageSize}.
Corrected by page=${correctedPageIndex}

Another solution would be to add a warning in the doc

Hmm. Your example is helpful and makes sense but technically since we switch to controlled pagination then we should be responsible to update page explicitly just like you did in your example code.

Also your sandbox example works as expected on online playground but locally I have much more props on grid then the example code, I have directly pasted the code from your example but still get the same issue. I am debugging the issue will update if find any.

Thanks.

[m4theushw]

Hmm. Your example is helpful and makes sense but technically since we switch to controlled pagination then we should be responsible to update page explicitly just like you did in your example code.

@cq-ubaid-khan I didn't understand what you meant here. The callback gives the new value and you're responsible for saving it in the state or not.

I'm wondering if we should display a warning when the page is out of the range. For example, it could be

@alexfauquette Do you mean when the user changes the page size to value that causes the current page to be out of range? If so, this's not an user's fault and warnings only appear in development mode. However, passing the reason for the page to have change might be useful.

---

Looking back to the issue, if users try to implement a server-side pagination as explained in this demo they will make two ajax requests when changing the page size to a value that also causes a page change. Here's an CodeSandbox showing the problem. Click 3 times in the next page button, then change the page size to 20. The effect is called twice. I think there would be a onPaginationParamsChange prop which is called when either the page or the page size change. This prop would give the updated value for both.

[alexfauquette]

@m4theushw Yes for both:

• onPaginationParamsChange seems to make more sense, because the size and the index are not independent.
• the warning must be only for developers when the pageIndex, pageSize is incompatible with the rowCount

Thanks for the CodeSandox which illustrates well the problem. I think onPaginationParamsChange control is the only way to solve it on our side.

[cq-ubaid-khan]

Hmm. Your example is helpful and makes sense but technically since we switch to controlled pagination then we should be responsible to update page explicitly just like you did in your example code.

@cq-ubaid-khan I didn't understand what you meant here. The callback gives the new value and you're responsible for saving it in the state or not.

I'm wondering if we should display a warning when the page is out of the range. For example, it could be

@alexfauquette Do you mean when the user changes the page size to value that causes the current page to be out of range? If so, this's not an user's fault and warnings only appear in development mode. However, passing the reason for the page to have change might be useful.

Looking back to the issue, if users try to implement a server-side pagination as explained in this demo they will make two ajax requests when changing the page size to an value that also causes a page change. Here's an CodeSandbox showing the problem. Click 3 times in the next page button, then change the page size to 20. The effect is called twice. I think there would be a onPaginationParamsChange prop which is called when either the page or the page size change. This prop would give the updated value for both.

Thanks for addressing this issue. I am still struggling with this issue and since you understand the problem, Is there any possible solutions to tackle this problem or do you have plan of adding onPaginationParamsChange prop ?

Thanks.

[flaviendelangle]

Concerning onPaginationParamsChange, I don't know how hard it will be to add it since the controlled pattern is very generic and pageSize / page are two distincts controlled props.

You can group the update with a Promise.resolve and add the constraints on the page: https://codesandbox.io/s/flamboyant-vaughan-zyg6g?file=/src/Demo.tsx
I think it should work fine but it's worth testing.