[Pagination] nextButton won't disable after providing float 'count' props #25151

fayzzzm · 2021-03-01T09:47:58Z

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

Current Behavior 😯

When passing float 'count' props to PaginationComponent bugs occur.

You can see our discussion with @oliviertassinari about this behavior in this link.

The 'nextbutton' is not disabled on the last page when count is provided with a float number.

<Pagination count={1.3} />

result: link to sandbox

Expected Behavior 🤔

PaginationComponent should return a warning when count is provided with a float number.

Steps to Reproduce 🕹

PaginationComponent proptypes uses number proptype for validating. Instead, I have changed to my custom proptype which warns the developer when he/she passes a float number as a proptype. Moreover, the component gives an error when it is not provided with a number at all.

Steps:

Change PaginationComponent count proptype with custom proptype

My build result: codesandbox link
Screenshot:

Context 🔦

I used the PaginationComponent whilst dividing the totalAmount into pages, of length 30. Sometimes this division resulted in float numbers which I passed to PaginationComponent thus I had a bug with the clickable next page button on the last page.

Your Environment 🌎

Using FireFox

`npx @material-ui/envinfo`

``` System: OS: macOS 11.2 Binaries: Node: 14.16.0 - /usr/local/bin/node Yarn: 1.22.10 - /usr/local/bin/yarn npm: 6.14.11 - /usr/local/bin/npm Browsers: Chrome: Not Found Edge: Not Found Firefox: 86.0 Safari: 14.0.3 npmPackages: @emotion/react: ^11.0.0 => 11.1.5 @emotion/styled: ^11.0.0 => 11.1.5 @material-ui/codemod: 5.0.0-alpha.24 @material-ui/core: 5.0.0-alpha.26 @material-ui/data-grid: 4.0.0-alpha.20 @material-ui/docs: 5.0.0-alpha.26 @material-ui/envinfo: 1.1.6 @material-ui/icons: 5.0.0-alpha.26 @material-ui/lab: 5.0.0-alpha.26 @material-ui/styled-engine: 5.0.0-alpha.25 @material-ui/styled-engine-sc: 5.0.0-alpha.25 @material-ui/styles: 5.0.0-alpha.26 @material-ui/system: 5.0.0-alpha.26 @material-ui/types: 5.1.7 @material-ui/unstyled: 5.0.0-alpha.26 @material-ui/utils: 5.0.0-alpha.26 @types/react: ^17.0.0 => 17.0.2 react: ^16.14.0 => 16.14.0 react-dom: ^16.14.0 => 16.14.0 styled-components: 5.2.1 typescript: ^4.1.2 => 4.2.2 ```

The text was updated successfully, but these errors were encountered:

oliviertassinari · 2021-03-02T12:07:38Z

@fayzzzm Thanks for opening this issue. I agree, I think that we should add an integer prototype. This is a quick POC:

diff --git a/docs/pages/api-docs/pagination.json b/docs/pages/api-docs/pagination.json
index 79dd3cb717..33cada86a1 100644
--- a/docs/pages/api-docs/pagination.json
+++ b/docs/pages/api-docs/pagination.json
@@ -27,7 +27,7 @@
     },
     "showFirstButton": { "type": { "name": "bool" } },
     "showLastButton": { "type": { "name": "bool" } },
-    "siblingCount": { "type": { "name": "number" }, "default": "1" },
+    "siblingCount": { "type": { "name": "custom", "description": "integer" }, "default": "1" },
     "size": {
       "type": {
         "name": "enum",
diff --git a/docs/scripts/buildApi.ts b/docs/scripts/buildApi.ts
index 223a644258..4fb7c2cd6e 100644
--- a/docs/scripts/buildApi.ts
+++ b/docs/scripts/buildApi.ts
@@ -32,7 +32,8 @@ import { pageToTitle } from 'docs/src/modules/utils/helpers';
 import createGenerateClassName from '@material-ui/styles/createGenerateClassName';
 import getStylesCreator from '@material-ui/styles/getStylesCreator';
 import { createMuiTheme } from '@material-ui/core/styles';
-import { getLineFeed, getUnstyledFilename } from './helpers';
+import { getLineFeed, getUnstyledFilename } from 'docs/scripts/helpers';
+import generatePropTypeDescription from 'docs/src/modules/utils/generatePropTypeDescription';

 const DEMO_IGNORE = LANGUAGES_IN_PROGRESS.map((language) => `-${language}.md`);

@@ -126,83 +127,6 @@ function isElementAcceptingRefProp(type: PropTypeDescriptor): boolean {
   return /^elementAcceptingRef/.test(type.raw);
 }

-function generatePropTypeDescription(type: PropTypeDescriptor): string | undefined {
-  switch (type.name) {
-    case 'custom': {
-      if (isElementTypeAcceptingRefProp(type)) {
-        return `element type`;
-      }
-      if (isElementAcceptingRefProp(type)) {
-        return `element`;
-      }
-      if (isRefType(type)) {
-        return `ref`;
-      }
-      if (type.raw === 'HTMLElementType') {
-        return `HTML element`;
-      }
-
-      const deprecatedInfo = getDeprecatedInfo(type);
-      if (deprecatedInfo !== false) {
-        return generatePropTypeDescription({
-          // eslint-disable-next-line react/forbid-foreign-prop-types
-          name: deprecatedInfo.propTypes,
-        } as any);
-      }
-
-      const chained = getChained(type);
-      if (chained !== false) {
-        return generatePropTypeDescription(chained.type);
-      }
-
-      return type.raw;
-    }
-
-    case 'shape':
-      return `{ ${Object.keys(type.value)
-        .map((subValue) => {
-          const subType = type.value[subValue];
-          return `${subValue}${subType.required ? '' : '?'}: ${generatePropTypeDescription(
-            subType,
-          )}`;
-        })
-        .join(', ')} }`;
-
-    case 'union':
-      return (
-        type.value
-          .map((type2) => {
-            return generatePropTypeDescription(type2);
-          })
-          // Display one value per line as it's better for visibility.
-          .join('<br>&#124;&nbsp;')
-      );
-    case 'enum':
-      return (
-        type.value
-          .map((type2) => {
-            return escapeCell(type2.value);
-          })
-          // Display one value per line as it's better for visibility.
-          .join('<br>&#124;&nbsp;')
-      );
-
-    case 'arrayOf': {
-      return `Array&lt;${generatePropTypeDescription(type.value)}&gt;`;
-    }
-
-    case 'instanceOf': {
-      if (type.value.startsWith('typeof')) {
-        return /typeof (.*) ===/.exec(type.value)![1];
-      }
-      return type.value;
-    }
-
-    default:
-      return type.name;
-  }
-}
-
 /**
  * Returns `null` if the prop should be ignored.
  * Throws if it is invalid.
diff --git a/docs/src/modules/utils/generatePropTypeDescription.ts b/docs/src/modules/utils/generatePropTypeDescription.ts
index 28297de96f..7fabdbb3f1 100644
--- a/docs/src/modules/utils/generatePropTypeDescription.ts
+++ b/docs/src/modules/utils/generatePropTypeDescription.ts
@@ -60,6 +60,10 @@ function isRefType(type: PropTypeDescriptor): boolean {
   return type.raw === 'refType';
 }

+function isIntegerPropType(type: PropTypeDescriptor): boolean {
+  return type.raw === 'integerPropType';
+}
+
 function isElementAcceptingRefProp(type: PropTypeDescriptor): boolean {
   return /^elementAcceptingRef/.test(type.raw);
 }
@@ -76,6 +80,9 @@ export default function generatePropTypeDescription(type: PropTypeDescriptor): s
       if (isRefType(type)) {
         return `ref`;
       }
+      if (isIntegerPropType(type)) {
+        return 'integer';
+      }
       if (type.raw === 'HTMLElementType') {
         return `HTML element`;
       }
diff --git a/packages/material-ui-utils/src/index.ts b/packages/material-ui-utils/src/index.ts
index 7f4d526553..6e5d3c5466 100644
--- a/packages/material-ui-utils/src/index.ts
+++ b/packages/material-ui-utils/src/index.ts
@@ -6,6 +6,7 @@ export { default as exactProp } from './exactProp';
 export { default as formatMuiErrorMessage } from './formatMuiErrorMessage';
 export { default as getDisplayName } from './getDisplayName';
 export { default as HTMLElementType } from './HTMLElementType';
+export { default as integerPropType } from './integerPropType';
 export { default as ponyfillGlobal } from './ponyfillGlobal';
 export { default as refType } from './refType';
 export { default as unstable_capitalize } from './capitalize';
diff --git a/packages/material-ui-utils/src/integerPropType.js b/packages/material-ui-utils/src/integerPropType.js
new file mode 100644
index 0000000000..53306f8314
--- /dev/null
+++ b/packages/material-ui-utils/src/integerPropType.js
@@ -0,0 +1,34 @@
+
+// IE 11 support
+function ponyfillIsInteger(x) {
+  // eslint-disable-next-line no-restricted-globals
+  return typeof x === 'number' && isFinite(x) && Math.floor(x) === x;
+}
+
+const isInteger = Number.isInteger || ponyfillIsInteger;
+
+function requiredInteger(props, propName) {
+  const propValue = props[propName];
+
+  if (propValue == null || !isInteger(propValue)) {
+    return new RangeError(`\`${propName}\` must be an integer. Instead, \`${propValue}\` was provided.`);
+  }
+
+  return null;
+}
+
+export default function integerPropType(props, propName) {
+  if (process.env.NODE_ENV === 'production') {
+    return null;
+  }
+
+  const propValue = props[propName];
+
+  if (propValue == null) {
+    return null;
+  }
+
+  return requiredInteger(props, propName);
+}
+
+integerPropType.isRequired = requiredInteger;
diff --git a/packages/material-ui/src/Pagination/Pagination.js b/packages/material-ui/src/Pagination/Pagination.js
index aff728a648..c621a3c1ca 100644
--- a/packages/material-ui/src/Pagination/Pagination.js
+++ b/packages/material-ui/src/Pagination/Pagination.js
@@ -1,6 +1,7 @@
 import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
+import { integerPropType } from '@material-ui/utils';
 import { withStyles, useThemeVariants } from '../styles';
 import usePagination from '../usePagination';
 import PaginationItem from '../PaginationItem';
@@ -202,7 +203,7 @@ Pagination.propTypes = {
    * Number of always visible pages before and after the current page.
    * @default 1
    */
-  siblingCount: PropTypes.number,
+  siblingCount: /* @typescript-to-proptypes-ignore */ integerPropType,
   /**
    * The size of the component.
    * @default 'medium'

While doing it, I have noticed that how we handle custom prop-types is inconsistent:

We could simplify the error messages. The component stack trace is often enough, we don't need to mention the name of the component for instance
We have duplicated generatePropTypeDescription
We could always use PropType as a suffix in the name to remove any potential confusion.
We can leverage if (process.env.NODE_ENV === 'production') { to reduce the size of the custom prop-types in production.

fayzzzm · 2021-03-02T13:56:54Z

The changes are great @oliviertassinari.

As you have mentioned about inconsistency:

Yes, in the error messages, there name of the component which handles this error shouldn't be mentioned, due to the stack trace.
Due to changes that you've made, you removed this.
What about the moments if we have several proptypes ? We could create an object PropTypeCustom which has properties that handle the function with the exact logic of validating custom proptype by fieldType | type.
Agree.

+  if (propValue == null || !isInteger(propValue)) {
+    return new RangeError(`\`${propName}\` must be an integer. Instead, \`${propValue}\` was provided.`);
+  }
+

I think we don't need propValue == null due to the default value that we could set in our component.
Reference to your code:

+  if (propValue == null) {
+    return null;
+  }
+
+  return requiredInteger(props, propName);

You are skipping the checking null value, so in requiredInteger we don't need to check it again

oliviertassinari · 2021-03-02T13:58:41Z

propValue == null is important, we can't remove it. Try without and should be able to understand why.

fayzzzm · 2021-03-02T16:31:32Z

Difference between next branch and fayzzzm.customPropType:

diff --git a/packages/material-ui-utils/src/index.ts b/packages/material-ui-utils/src/
index.ts
index 7f4d526553..11be6204d4 100644
--- a/packages/material-ui-utils/src/index.ts
+++ b/packages/material-ui-utils/src/index.ts
@@ -31,3 +31,4 @@ export {
 } from './scrollLeft';
 export { default as usePreviousProps } from './usePreviousProps';
 export { default as visuallyHidden } from './visuallyHidden';
+export { default as integerPropType } from './integerPropType';
\ No newline at end of file
diff --git a/packages/material-ui-utils/src/integerPropType.js b/packages/material-ui-
utils/src/integerPropType.js
new file mode 100644
index 0000000000..b834931130
--- /dev/null
+++ b/packages/material-ui-utils/src/integerPropType.js
@@ -0,0 +1,33 @@
+// IE 11 support
+function ponyfillIsInteger(x) {
+  // eslint-disable-next-line no-restricted-globals
+  return typeof x === 'number' && isFinite(x) && Math.floor(x) === x;
+}
+
+const isInteger = Number.isInteger || ponyfillIsInteger;
+
+function requiredInteger(props, propName) {
+  const propValue = props[propName];
+
+  if (propValue == null || !isInteger(propValue)) {
+    return new RangeError(`\`${propName}\` must be an integer. Instead, \`${propValue}\` was provided.`);
+  }
+
+  return null;
+}
+
+export default function integerPropType(props, propName) {
+  if (process.env.NODE_ENV === 'production') {
+    return null;
+  }
+
+  const propValue = props[propName];
+
+  if (propValue == null) {
+    return null;
+  }
+
+  return requiredInteger(props, propName);
+}
+
+integerPropType.isRequired = requiredInteger;
\ No newline at end of file
diff --git a/packages/material-ui/src/Pagination/Pagination.js b/packages/material-ui/src/Pagination/Pagination.js
index 79bb44d0f0..5b2f4c464c 100644
--- a/packages/material-ui/src/Pagination/Pagination.js
+++ b/packages/material-ui/src/Pagination/Pagination.js
@@ -2,7 +2,7 @@ import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled';
-import { deepmerge } from '@material-ui/utils';
+import { deepmerge, integerPropType } from '@material-ui/utils';
 import useThemeProps from '../styles/useThemeProps';
 import paginationClasses, { getPaginationUtilityClass } from './paginationClasses';
 import usePagination from '../usePagination';
@@ -164,7 +164,7 @@ Pagination.propTypes = {
    * The total number of pages.
    * @default 1
    */
-  count: PropTypes.number,
+  count: integerPropType,
   /**
    * The page selected by default when the component is uncontrolled.
    * @default 1

These changes are the same as yours, but if you look up to this codesandbox link, you can notice that, with null passing, there is no error occur and pagination doesn't set by default count = 1.

Same for removing propValue == null ||

diff --git a/packages/material-ui-utils/src/integerPropType.js b/packages/material-ui-
utils/src/integerPropType.js
index b834931130..1d2910d835 100644
--- a/packages/material-ui-utils/src/integerPropType.js
+++ b/packages/material-ui-utils/src/integerPropType.js
@@ -9,7 +9,7 @@ const isInteger = Number.isInteger || ponyfillIsInteger;
 function requiredInteger(props, propName) {
   const propValue = props[propName];
 
-  if (propValue == null || !isInteger(propValue)) {
+  if (!isInteger(propValue)) {
     return new RangeError(`\`${propName}\` must be an integer. Instead, \`${propValue
}\` was provided.`);
   }

Nothing changes at all. link to codesandbox

By looking through all of these things, I've noticed that we should check our propValue === undefined at the beginning by approving that, the user is not passing anything and the default value of counter will be 1. So, I've made the changes like this:

diff --git a/packages/material-ui-utils/src/integerPropType.js b/packages/material-ui-
utils/src/integerPropType.js
index b834931130..962105ac9d 100644
--- a/packages/material-ui-utils/src/integerPropType.js
+++ b/packages/material-ui-utils/src/integerPropType.js
@@ -23,7 +23,7 @@ export default function integerPropType(props, propName) {
 
   const propValue = props[propName];
 
-  if (propValue == null) {
+  if (propValue === undefined) {
     return null;
   }

So now, when the user is trying to pass null, he/she gets an error:

Link to all CodeSandbox:

Custom proptype as yours: link to codesandbox
Custom proptype without null check at integerPropType function: link to codesandbox
Custom proptype but instead of checking null at the beginning, we will look for undefined: link to codesandbox

Can you look up to all of these codesandboxes @oliviertassinari ?

oliviertassinari · 2021-03-02T17:25:32Z

It should be == null see https://github.com/facebook/prop-types/blob/d62a775b78085e38805d34f27295798bcf54987c/factoryWithTypeCheckers.js#L209
We could breakdown the effort into smaller pull requests.
- start by fixing the generatePropTypeDescription duplication [docs] Remove dead generatePropTypeDescription method #25188
- add the integerPropType following the best practices, test cases, etc. Use it in a single place. [utils] Add a new integer propType #25224
- increase the adoption of the new prop-types everywhere it's relevant in the components.
- update the other prop-types to follow the best practices, mostly in the naming and error consistency conversions.

fayzzzm · 2021-03-02T17:40:08Z

@oliviertassinari they use == not === and then in the scope of if they validate it again with ===:

if (props[propName] === null) {
  return new PropTypeError('The ' + location + ' `' + propFullName + '` is marked as required ' + ('in `' + componentName + '`, but its value is `null`.'));
}

In our case:

We just validate by ==, and then we don't do anything. (You can correct me if am wrong).
We can easily pass null value which is incorrect.

Agree with the breakdown to smaller PRs', will definitely do that.

fayzzzm · 2021-03-04T07:53:28Z

@oliviertassinari could you check this codesandbox link please. It is for integerPropType, you can find it in customPropType folder.

oliviertassinari · 2021-03-04T15:58:23Z

@fayzzzm What should be reviewed? Why not work on pull requests directly? Could we start by removing the generatePropTypeDescription duplication?

fayzzzm · 2021-03-04T16:26:01Z

@oliviertassinari Sorry for that, here is PR link

oliviertassinari · 2021-03-04T18:01:51Z

@fayzzzm Perfect 👌

fayzzzm · 2021-03-04T18:23:25Z

I will move forward by tackling the next PR, which you offered:

add the integerPropType following the best practices, test cases, etc. Use it in a single place.

Btw, thanks for the help @oliviertassinari!

oliviertassinari · 2021-03-04T20:11:18Z

@fayzzzm Sounds like a plan. It might be simpler to wait for #25188 to be merged. You will need to changes.

oliviertassinari · 2021-03-06T19:48:40Z

@fayzzzm I have updated #25151 (comment). Do you want to move forward with the second step? I had a look at https://codesandbox.io/s/romantic-northcutt-hfw5l?file=/src/customPropType/index.js. A couple of thoughts:

It does seem weird that PropTypes.number allows null natively. I could find a couple of issues open about this problem in the prop-types repo, e.g. Add isDefined and isNotNull parallels to isRequired facebook/prop-types#90. Looks like a legacy debt. null isn't allowed in TypeScript (?: is | undefined) and prevents the default value to trigger.
How about we try moving forward without skipNull? I think that testing propValue === undefined should indeed be enough.
For the process.env.NODE_ENV === "production", AFAIK, it can only work inside an export module. We can't branch in the exports
Keeping integerPropType.js flat inside the utils package should be enough.
I wonder about what should be the best error message. If you look at the other custom prop-types we have, the way we write the error message is completely inconsistent. I would suggest copying what prop-types is doing.

I think that we can move directly to a PR.

fayzzzm · 2021-03-06T20:45:20Z

What if to pass default value in our typescript function as: validate (skipNull: boolean = true) ? But as we've seen in practice, when you don't pass props in a component, you'll get undefined when you destruct it (if it's not required), so PropType should really prevent of passing null value when it's not required (validation should pass only on undefined value). So i will move forward with validation propValue === undefined.
I think, if we combine them together (to inline all codes inside integerPropType.js), there shouldn't be any problem as we'll check process.env.NODE_ENV === "production" inside the function.
Agree.
I will copy prop-types error message and in the future we could discuss about some changes.

I will move directly to PR and soon as possible will send.
@oliviertassinari thanks for assisting me! I appreciate it.

EDIT: The same issue could be found on airbnb-proptypes (link to integer.js code). I think, they've written it for the reason when a developer won't make destruct, but instead, will use conversion of

prop.yourNumberProp || defaultValue

But in our case, we are passing the default value inside destruct object so, null won't be changed. But as mentioned, it should be either an integer | undefined

fayzzzm · 2021-03-07T00:45:13Z

@oliviertassinari here is the PR link.

oliviertassinari · 2021-03-09T21:56:24Z

I'm wondering if we should consider this issue fixed. We fixed one instance of it.

fayzzzm added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 1, 2021

fayzzzm mentioned this issue Mar 4, 2021

[docs] Remove dead generatePropTypeDescription method #25188

Merged

1 task

fayzzzm mentioned this issue Mar 7, 2021

[utils] Add a new integer propType #25224

Merged

1 task

eps1lon linked a pull request Mar 7, 2021 that will close this issue

[utils] Add a new integer propType #25224

Merged

1 task

oliviertassinari closed this as completed in #25224 Mar 9, 2021

oliviertassinari reopened this Mar 9, 2021

oliviertassinari closed this as completed Mar 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pagination] nextButton won't disable after providing float 'count' props #25151

[Pagination] nextButton won't disable after providing float 'count' props #25151

fayzzzm commented Mar 1, 2021

oliviertassinari commented Mar 2, 2021 •

edited

fayzzzm commented Mar 2, 2021 •

edited

oliviertassinari commented Mar 2, 2021

fayzzzm commented Mar 2, 2021

oliviertassinari commented Mar 2, 2021 •

edited

fayzzzm commented Mar 2, 2021 •

edited

fayzzzm commented Mar 4, 2021 •

edited

oliviertassinari commented Mar 4, 2021

fayzzzm commented Mar 4, 2021 •

edited

oliviertassinari commented Mar 4, 2021

fayzzzm commented Mar 4, 2021 •

edited

oliviertassinari commented Mar 4, 2021

oliviertassinari commented Mar 6, 2021 •

edited

fayzzzm commented Mar 6, 2021 •

edited

fayzzzm commented Mar 7, 2021

oliviertassinari commented Mar 9, 2021 •

edited

[Pagination] nextButton won't disable after providing float 'count' props #25151

[Pagination] nextButton won't disable after providing float 'count' props #25151

Comments

fayzzzm commented Mar 1, 2021

Current Behavior 😯

Expected Behavior 🤔

Steps to Reproduce 🕹

Context 🔦

Your Environment 🌎

oliviertassinari commented Mar 2, 2021 • edited

fayzzzm commented Mar 2, 2021 • edited

oliviertassinari commented Mar 2, 2021

fayzzzm commented Mar 2, 2021

oliviertassinari commented Mar 2, 2021 • edited

fayzzzm commented Mar 2, 2021 • edited

fayzzzm commented Mar 4, 2021 • edited

oliviertassinari commented Mar 4, 2021

fayzzzm commented Mar 4, 2021 • edited

oliviertassinari commented Mar 4, 2021

fayzzzm commented Mar 4, 2021 • edited

oliviertassinari commented Mar 4, 2021

oliviertassinari commented Mar 6, 2021 • edited

fayzzzm commented Mar 6, 2021 • edited

fayzzzm commented Mar 7, 2021

oliviertassinari commented Mar 9, 2021 • edited

oliviertassinari commented Mar 2, 2021 •

edited

fayzzzm commented Mar 2, 2021 •

edited

oliviertassinari commented Mar 2, 2021 •

edited

fayzzzm commented Mar 2, 2021 •

edited

fayzzzm commented Mar 4, 2021 •

edited

fayzzzm commented Mar 4, 2021 •

edited

fayzzzm commented Mar 4, 2021 •

edited

oliviertassinari commented Mar 6, 2021 •

edited

fayzzzm commented Mar 6, 2021 •

edited

oliviertassinari commented Mar 9, 2021 •

edited