Permalink
Browse files

Core: Improve isNumeric logic and test coverage

Also add back accidentally deleted comments about the implementation.

Fixes gh-2780
Ref gh-2663
Ref gh-2781
Closes gh-2827
  • Loading branch information...
1 parent e04e246 commit 7103d8ef47e04a4cf373abee0e8bfa9062fd616f @stevemao stevemao committed with gibson042 Jan 14, 2016
Showing with 28 additions and 3 deletions.
  1. +5 −1 src/core.js
  2. +23 −2 test/unit/core.js
View
@@ -217,7 +217,11 @@ jQuery.extend( {
// that can be coerced to finite numbers (gh-2662)
var type = jQuery.type( obj );
return ( type === "number" || type === "string" ) &&
- ( obj - parseFloat( obj ) + 1 ) >= 0;
+
+ // parseFloat NaNs numeric-cast false positives ("")
+ // ...but misinterprets leading-number strings, particularly hex literals ("0x...")
+ // subtraction forces infinities to NaN
+ !isNaN( obj - parseFloat( obj ) );
},
isPlainObject: function( obj ) {
View
@@ -449,7 +449,7 @@ QUnit.test( "isFunction", function( assert ) {
} );
QUnit.test( "isNumeric", function( assert ) {
- assert.expect( 38 );
+ assert.expect( 41 );
var t = jQuery.isNumeric,
ToString = function( value ) {
@@ -464,8 +464,29 @@ QUnit.test( "isNumeric", function( assert ) {
assert.ok( t( -16 ), "Negative integer number" );
assert.ok( t( 0 ), "Zero integer number" );
assert.ok( t( 32 ), "Positive integer number" );
+
+ if ( +"0b1" === 1 ) {
+ assert.ok( t( "0b111110" ), "Binary integer literal string" ); // jshint ignore:line
+ } else {
+ assert.ok( true, "Browser does not support binary integer literal" );
+ }
+
assert.ok( t( "040" ), "Octal integer literal string" );
+
assert.ok( t( "0xFF" ), "Hexadecimal integer literal string" );
+
+ if ( +"0b1" === 1 ) {
+ assert.ok( t( 0b111110 ), "Binary integer literal" ); // jshint ignore:line
@mgol
mgol Jan 25, 2016 Member

This is a syntax error in non-ES6 browsers; it's not enough to wrap it in a simple if... I assume you wanted to pass strings here?

@stevemao
stevemao Feb 1, 2016 Contributor

This is a syntax error in non-ES6 browsers; it's not enough to wrap it in a simple if

@mgol Why? +"0b1" === 1 doesn't return falsy value in non-ES6 browsers?

@mgol
mgol Feb 1, 2016 Member

0b111110 is a syntax error in browsers that didn't implement the binary notation so this if won't even get evaluated, the whole file will immediately throw a syntax error. See it by yourself: http://swarm.jquery.org/job/2146.

The fix has landed in 2868db0.

@mgol
mgol Feb 1, 2016 Member

Also, even in ES6 browsers this test didn't make sense as there 0b111110 === 62 so this line is equivalent to:

assert.ok( t( 62 ), "Binary integer literal" );

We weren't really testing the binary notation here, to the browser they're equivalent after interpreting the number literal. What we really wanted to test is t( "0b111110" ) - a string gets passed to the function directly and that's how we can check what jQuery will do with a string that represents a number in a binary notation.

@stevemao
stevemao Feb 1, 2016 Contributor

Oh right... Thanks @mgol :)

+ } else {
+ assert.ok( true, "Browser does not support binary integer literal" );
+ }
+
+ if ( +"0o1" === 1 ) {
+ assert.ok( t( 0o76 ), "Octal integer literal" ); // jshint ignore:line
+ } else {
+ assert.ok( true, "Browser does not support octal integer literal" );
+ }
+
assert.ok( t( 0xFFF ), "Hexadecimal integer literal" );
assert.ok( t( "-1.6" ), "Negative floating point string" );
assert.ok( t( "4.536" ), "Positive floating point string" );
@@ -475,7 +496,7 @@ QUnit.test( "isNumeric", function( assert ) {
assert.ok( t( 8e5 ), "Exponential notation" );
assert.ok( t( "123e-2" ), "Exponential notation string" );
- assert.equal( t( new ToString( "42" ) ), false, "Custom .toString returning number" );
+ assert.equal( t( new ToString( "42" ) ), false, "Only limited to strings and numbers" );
assert.equal( t( "" ), false, "Empty string" );
assert.equal( t( " " ), false, "Whitespace characters string" );
assert.equal( t( "\t\t" ), false, "Tab characters string" );

0 comments on commit 7103d8e

Please sign in to comment.