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.

Sign up for GitHub

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

Use parseInt instead of | operator for integer validation #147

Closed
wants to merge 1 commit into from
Closed

Use parseInt instead of | operator for integer validation #147

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/number.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import isAbsent from './util/isAbsent';

let isNaN = value => value != +value

let isInteger = val => isAbsent(val) || val === (val | 0)
let isInteger = val => isAbsent(val) || val === parseInt(val, 10)
Copy link
Owner

Choose a reason for hiding this comment

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

parseInt isn't the right thing, here we don't want to coerce the value into a number. The reason the original code doesn't work with large ints, btw, is that JS really only has 32bit ints and the bitwise operators first convert the float to an int then truncate it. This leads to overflows in the large number case.

I'm a bit torn, the original method is more "correct" even if it does seemingly odd things. If we were gonna change it tho, we should use the Number.isInteger logic e.g. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isInteger#Polyfill

Copy link
Author

Choose a reason for hiding this comment

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

I think the coercion is fine if strict equality is being used (===)

Copy link
Author

Choose a reason for hiding this comment

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

But any approach that makes false negatives go away is fine by me 👍

Copy link

@sebpiq sebpiq Nov 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

+1 for @jozanza approach, parseInt + strict equality is everything that's needed, and it's more readable.

Copy link
Author

@jozanza jozanza Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I finally realized the flaw of the parseInt approach:

9999999999999999999.9 === parseInt(9999999999999999999.9, 10)
// true

Numbers can't be that large in JS, so it's basically 10000000000000000000 === 10000000000000000000 after a certain point.

I'll work on adding the Number.isInteger logic to this PR when I get a chance @jquense


export default function NumberSchema() {
if (!(this instanceof NumberSchema))
Expand Down