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

Fix parseInt and parseFloat #402

Merged
merged 1 commit into from
Jun 13, 2016

Conversation

ValeryVS
Copy link
Contributor

parsing with undefined, null or NaN should return NaN

@@ -1980,9 +1980,15 @@
parseInt = (function (origParseInt) {
var hexRegex = /^[\-+]?0[xX]/;
return function parseInt(str, radix) {
var string = trim(str);
var defaultedRadix = $Number(radix) || (hexRegex.test(string) ? 16 : 10);
return origParseInt(string, defaultedRadix);
Copy link
Member

Choose a reason for hiding this comment

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

Since this calls into the original function, shouldn't it already return NaN?

In which browsers/engines does the existing shim not do this?

Copy link
Member

Choose a reason for hiding this comment

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

Also, parseInt('undefined') should also be NaN, so since parseInt changes all input to strings, checking the typeof isn't very helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In firefox 17 parseInt(null) and parseInt(undefined) are throw "can't convert undefined to object".
But parseInt(NaN) is ok. Need some testing.

Probably that is because of
var trim = call.bind(String.prototype.trim);

Copy link
Member

Choose a reason for hiding this comment

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

aha, ok - in that case, I probably just need trim(String(str))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this will work.
Also, this problem is for undefined and null only.

Function.prototype.call.bind(String.prototype.trim)(undefined)
throws Uncaught TypeError: String.prototype.trim called on null or undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trim will convert any other variable to String, anyway. So, trim(String(str)) seems to be nice solution.

@ljharb
Copy link
Member

ljharb commented Jun 10, 2016

If you want to change your implementation diff to be just what we discussed, and then rebase everything down to a commit on latest master, I'd love to merge it :-)

convert argument to String to prevent errors when functions called with undefined or null
@ValeryVS
Copy link
Contributor Author

Done.

@ljharb
Copy link
Member

ljharb commented Jun 13, 2016

@ValeryVS hmm, I'm checking in FF 17, and even on master, all of your added tests are passing. Which version of es5-shim are you using? Can you compare the latest?

Can you create a jsfiddle or equivalent that reproduces the failure?

@ValeryVS
Copy link
Contributor Author

Here is plunk
https://plnkr.co/edit/OjdaQ9xbq66yg4zmxlFR

es5-shim file from master, but "if" conditions near parseInt and parseFloat are commented out.
So, even in modern browsers, there will be
"TypeError: String.prototype.trim called on null or undefined"
in console.

@zloirock
Copy link

@ljharb I wrote you about this bug ~ half a year ago. One new report - zloirock/core-js#208 (comment)

@ValeryVS
Copy link
Contributor Author

ValeryVS commented Jun 13, 2016

FF 17 throws
TypeError: can't convert undefined to object
es5-shim.js
Row: 1983

@ljharb
Copy link
Member

ljharb commented Jun 13, 2016

@zloirock i don't see an issue filed - random comments on random threads invariably get lost.

@ValeryVS I get a 404 on that link.

@ValeryVS
Copy link
Contributor Author

@ljharb
You, probably open link to script from error.

This link to plunk from my first comment works

Here is plunk
https://plnkr.co/edit/OjdaQ9xbq66yg4zmxlFR

To see the error, you must open browser console and ther click the "run" button.

@ljharb
Copy link
Member

ljharb commented Jun 13, 2016

@ValeryVS aha, got it :-) thank you!

@ljharb ljharb merged commit 406f0ff into es-shims:master Jun 13, 2016
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.

None yet

3 participants