Skip to content

Conversation

timfish
Copy link

@timfish timfish commented May 20, 2019

Removes the need to polyfill Number.parseInt|parseFloat by simply using the top level parseInt|parseFloat.

Fixes #162

@coveralls
Copy link

coveralls commented May 20, 2019

Coverage Status

Coverage increased (+0.3%) to 97.531% when pulling 99dcddd on timfish:master into 65c39be on NaturalIntelligence:master.

@amitguptagwl
Copy link
Member

Sorry I didn't understand that how are you ensuring it to work on IE?

@timfish
Copy link
Author

timfish commented May 21, 2019

I changed Number.parseFloat to simply use parseFloat. parseFloat exists even on IE6.

@amitguptagwl
Copy link
Member

We knowingly placed them to fix an error when we found them not present in some browser. You can probably check in closed issues for more detail.

@timfish
Copy link
Author

timfish commented Jun 1, 2019

I don't think you've understood my changes correctly.

Number.parseInt does not exist in older versions of IE.

To fix this, it looks like a polyfill was added which copies parseInt from global window Number.parseInt = window.parseInt.

But the above polyfill has mistakes in it. #162 highlights this. window does not exist in all JavaScript contexts.

Rather than use Number.parseInt you should just be using parseInt on its own which works in every browser released in the lat 10 years.

This PR fixed the issue with window and still works in every browser.

For best browser support, you should be using parseInt over window.parseInt or Number.parseInt

@amitguptagwl
Copy link
Member

I'm fine with having in this case

if (!Number.parseInt) {
  Number.parseInt = parseInt;
}
if (!Number.parseFloat) {
  Number.parseFloat = parseFloat;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Does not work in IE11 Web Worker due to use of window

3 participants