Skip to content

modify isSafeInteger polyfill #1026

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DiligentYe
Copy link

Number.MAX_SAFE_INTEGER is defined by ES6, so Number.MAX_SAFE_INTEGER should be replaced by Math.pow(2, 53) - 1 there.

@legodude17
Copy link

Don't you mean isSafeInteger and not isSaftInteger?

@DiligentYe
Copy link
Author

Oh, sorry! I mean isSafeInteger!

@legodude17
Copy link

That makes much more sense. 😄

@getify getify changed the title modify isSaftInteger polyfill modify isSafeInteger polyfill Aug 15, 2017
@getify
Copy link
Owner

getify commented Aug 15, 2017

The assumption (though not stated) is that this polyfill would have relied on Number.MAX_SAFE_INTEGER having been defined, too. I think most usages would have done that, so I think it's safe. Perhaps at most we just need a code comment mentioning that assumption, rather than duplicating Math.pow(..) logic.

@getify getify force-pushed the master branch 6 times, most recently from 5cb284a to 8af48e2 Compare August 28, 2019 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants