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.

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

TablePagination deprecated props in 4.x.x introduced breaking changes #27192

Closed
2 tasks done
dhallstr opened this issue Jul 8, 2021 · 9 comments
Closed
2 tasks done
Assignees
Labels
bug 🐛 Something doesn't work component: table This is the name of the generic UI component, not the React module! ready to take Help wanted. Guidance available. There is a high chance the change will be accepted v5.x migration

Comments

@dhallstr
Copy link

dhallstr commented Jul 8, 2021

While prop deprecation is intended to be a non-breaking change, there is a corner case in TablePagination where renaming the onChangePage prop breaks existing code. Specifically, using TablePagination with a custom ActionsComponent is broken because the prop provided to the custom ActionsComponent has changed from onChangePage -> onPageChange.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

TablePagination no longer passes the custom ActionsComponent an onChangePage prop, causing the app to crash.

Expected Behavior 🤔

TablePagination should pass both onChangePage and onPageChange props to the ActionsComponent to support both the deprecated prop and the new prop.

Steps to Reproduce 🕹

The following CodeSandbox live examples contain exactly the same code, just using different MUI versions to demonstrate the breaking changes introduced in v4.12.0.

Working CodeSandbox with MUI v4.11.4: https://codesandbox.io/s/inspiring-jepsen-n2eyf?file=/src/Demo.tsx
Broken CodeSandbox with MUI v4.12.1: https://codesandbox.io/s/polished-butterfly-ym3u5

Steps:

  1. Render a <TablePagination> component with the deprecated onChangePage prop and a custom ActionsComponent prop
  2. Within the custom ActionsComponent, try calling props.onChangePage that worked prior to v4.12.0
  3. Click the Next or Previous button rendered within the custom ActionsComponent
  4. Notice that onChangePage is undefined, causing the app to crash.

Context 🔦

Your Environment 🌎

`npx @material-ui/envinfo`
System:
    OS: Windows 10 10.0.18363
  Binaries:
    Node: 14.16.0
    Yarn: Not Found
    npm: 6.14.13
  Browsers:
    Chrome: 91.0.4472.124
    Edge: Spartan (44.18362.1593.0)
  npmPackages:
    @material-ui/core: 4.12.1
    @material-ui/icons:  4.11.2
    @material-ui/styles:  4.11.4
    @material-ui/system:  4.12.1
    @material-ui/types:  5.1.0
    @material-ui/utils:  4.11.2
    @types/react:  17.0.13
    react: ^16.13.1 => 16.14.0
    react-dom: ^16.13.1 => 16.14.0
    typescript:  2.9.2
@dhallstr dhallstr added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 8, 2021
@oliviertassinari
Copy link
Member

See this thread #23789 (comment)

@mnajdova
Copy link
Member

mnajdova commented Jul 9, 2021

How about we do this diff:

index b027226200..e18d6edae2 100644
--- a/packages/material-ui/src/TablePagination/TablePagination.d.ts
+++ b/packages/material-ui/src/TablePagination/TablePagination.d.ts
@@ -32,7 +32,7 @@ export interface TablePaginationTypeMap<P, D extends React.ElementType> {
        * @deprecated Use the onPageChange prop instead.
        */
       onChangePage?: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
-      onPageChange: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
+      onPageChange?: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
       /**
        * Callback fired when the number of rows per page is changed.
        *
diff --git a/packages/material-ui/src/TablePagination/TablePagination.js b/packages/material-ui/src/TablePagination/TablePagination.js
index f056923360..dc4b85aed8 100644
--- a/packages/material-ui/src/TablePagination/TablePagination.js
+++ b/packages/material-ui/src/TablePagination/TablePagination.js
@@ -168,6 +168,7 @@ const TablePagination = React.forwardRef(function TablePagination(props, ref) {
             ...nextIconButtonProps,
           }}
           onPageChange={onChangePage}
+          onChangePage={onChangePage}
           page={page}
           rowsPerPage={rowsPerPage}
         />
@@ -263,7 +264,7 @@ TablePagination.propTypes = {
    * @param {object} event The event source of the callback.
    * @param {number} page The page selected.
    */
-  onPageChange: PropTypes.func.isRequired,
+  onPageChange: PropTypes /* @typescript-to-proptypes-ignore */.func.isRequired,
   /**
    * Callback fired when the number of rows per page is changed.
    *diff --git a/packages/material-ui/src/TablePagination/TablePaginationActions.d.ts b/packages/material-ui/src/TablePagination/TablePaginationActions.d.ts
index 17d76f48ea..6ebfc5f355 100644
--- a/packages/material-ui/src/TablePagination/TablePaginationActions.d.ts
+++ b/packages/material-ui/src/TablePagination/TablePaginationActions.d.ts
@@ -5,7 +5,9 @@ export interface TablePaginationActionsProps extends React.HTMLAttributes<HTMLDi
   backIconButtonProps?: Partial<IconButtonProps>;
   count: number;
   nextIconButtonProps?: Partial<IconButtonProps>;
-  onPageChange: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
+  // @deprecated Use the onPageChange prop instead.
+  onChangePage?: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
+  onPageChange?: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
   page: number;
   rowsPerPage: number;
 }
diff --git a/packages/material-ui/src/TablePagination/TablePaginationActions.js b/packages/material-ui/src/TablePagination/TablePaginationActions.js
index bbe0a95f0e..f7ddc51791 100644
--- a/packages/material-ui/src/TablePagination/TablePaginationActions.js
+++ b/packages/material-ui/src/TablePagination/TablePaginationActions.js
@@ -13,7 +13,8 @@ const TablePaginationActions = React.forwardRef(function TablePaginationActions(
     backIconButtonProps,
     count,
     nextIconButtonProps,
-    onPageChange,
+    onPageChange: onPageChangeProp,
+    onChangePage: onChangePageProp,
     page,
     rowsPerPage,
     ...other
@@ -21,12 +22,14 @@ const TablePaginationActions = React.forwardRef(function TablePaginationActions(

   const theme = useTheme();

+  const onPageChange = onPageChangeProp || onChangePageProp;
+
   const handleBackButtonClick = (event) => {
-    onPageChange(event, page - 1);
+    onPageChange?.(event, page - 1);
   };

   const handleNextButtonClick = (event) => {
-    onPageChange(event, page + 1);
+    onPageChange?.(event, page + 1);
   };

   return (

It should solve both the TS and the JS errors.

@oliviertassinari
Copy link
Member

Two notes

  • Is the onPageChange?.(event, page - 1); change required (I have changed it from onPageChange && onPageChange(event, page - 1);)
  • We still need to make sure onPageChange is displayed as required in the docs. We can override the generated prop-types. While we are still doing TS -> prop-types -> docs, we can customize it. But once we do TS -> docs for the generation. The types change will probably not be possible.

@mnajdova

This comment has been minimized.

@oliviertassinari oliviertassinari added component: table This is the name of the generic UI component, not the React module! v4.x regression A bug, but worse and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 9, 2021
@mnajdova
Copy link
Member

mnajdova commented Jul 9, 2021

Is the onPageChange?.(event, page - 1); change required (I have changed it from onPageChange && onPageChange(event, page - 1);)

I've added it as TypeScript is not making the params required.

We still need to make sure onPageChange is displayed as required in the docs. We can override the generated prop-types. While we are still doing TS -> prop-types -> docs, we can customize it. But once we do TS -> docs for the generation. The types change will probably not be possible.

I've updated #27192 (comment) to include this.

@medmin
Copy link

medmin commented Jul 10, 2021

OK, I am using new prop, onPageChange, but I've got warning in the console: Warning: Failed prop type: The prop onChangePageis marked as required inForwardRef(TablePagination), but its value is undefined.

It seems your documentation is for v5, not for the current version, i.e., v4

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Jul 12, 2021
@rolandjlevy
Copy link

@oliviertassinari thanks for helping me resolve the problem I was having. It's working now 😀

@Gorett

This comment has been minimized.

@eps1lon
Copy link
Member

eps1lon commented Aug 11, 2021

Closed in #27407

@eps1lon eps1lon closed this as completed Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: table This is the name of the generic UI component, not the React module! ready to take Help wanted. Guidance available. There is a high chance the change will be accepted v5.x migration
Projects
None yet
Development

No branches or pull requests

7 participants