feat: reworking how numbers are parsed (#104)

BREAKING CHANGE: strings that fail `Number.isSafeInteger()` are no longer coerced into numbers.
yargs · Oct 5, 2017 · fba00eb · fba00eb
1 parent d137227
commit fba00eb
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 15 deletions.
diff --git a/index.js b/index.js
@@ -270,9 +270,7 @@ function parse (args, opts) {
         }
       }
     } else {
-      argv._.push(
-        flags.strings['_'] || !isNumber(arg) ? arg : Number(arg)
-      )
+      argv._.push(maybeCoerceNumber('_', arg))
     }
   }
 
@@ -348,10 +346,8 @@ function parse (args, opts) {
   function setArg (key, val) {
     unsetDefaulted(key)
 
-    if (/-/.test(key) && !(flags.aliases[key] && flags.aliases[key].length) && configuration['camel-case-expansion']) {
-      var c = camelCase(key)
-      flags.aliases[key] = [c]
-      newAliases[c] = true
+    if (/-/.test(key) && configuration['camel-case-expansion']) {
+      addNewAlias(key, camelCase(key))
     }
 
     var value = processValue(key, val)
@@ -396,17 +392,23 @@ function parse (args, opts) {
     }
   }
 
+  function addNewAlias (key, alias) {
+    if (!(flags.aliases[key] && flags.aliases[key].length)) {
+      flags.aliases[key] = [alias]
+      newAliases[alias] = true
+    }
+    if (!(flags.aliases[alias] && flags.aliases[alias].length)) {
+      addNewAlias(alias, key)
+    }
+  }
+
   function processValue (key, val) {
     // handle parsing boolean arguments --foo=true --bar false.
     if (checkAllAliases(key, flags.bools) || checkAllAliases(key, flags.counts)) {
       if (typeof val === 'string') val = val === 'true'
     }
 
-    var value = val
-    if (!checkAllAliases(key, flags.strings) && !checkAllAliases(key, flags.coercions)) {
-      if (isNumber(val)) value = Number(val)
-      if (!isUndefined(val) && !isNumber(val) && checkAllAliases(key, flags.numbers)) value = NaN
-    }
+    var value = maybeCoerceNumber(key, val)
 
     // increment a count given as arg (either no value or value parsed as boolean)
     if (checkAllAliases(key, flags.counts) && (isUndefined(value) || typeof value === 'boolean')) {
@@ -421,6 +423,14 @@ function parse (args, opts) {
     return value
   }
 
+  function maybeCoerceNumber (key, value) {
+    if (!checkAllAliases(key, flags.strings) && !checkAllAliases(key, flags.coercions)) {
+      const shouldCoerceNumber = isNumber(value) && configuration['parse-numbers'] && (Number.isSafeInteger(parseInt(value)))
+      if (shouldCoerceNumber || (!isUndefined(value) && checkAllAliases(key, flags.numbers))) value = Number(value)
+    }
+    return value
+  }
+
   // set args from config.json file, this should be
   // applied last so that defaults can be applied.
   function setConfig (argv) {
@@ -602,7 +612,9 @@ function parse (args, opts) {
         flags.aliases[key].concat(key).forEach(function (x) {
           if (/-/.test(x) && configuration['camel-case-expansion']) {
             var c = camelCase(x)
-            flags.aliases[key].push(c)
+            if (flags.aliases[key].indexOf(c) === -1) {
+              flags.aliases[key].push(c)
+            }
             newAliases[c] = true
           }
         })
@@ -664,7 +676,6 @@ function parse (args, opts) {
   }
 
   function isNumber (x) {
-    if (!configuration['parse-numbers']) return false
     if (typeof x === 'number') return true
     if (/^0x[0-9a-f]+$/i.test(x)) return true
     return /^[-+]?(?:\d+(?:\.\d*)?|\.\d+)(e[-+]?\d+)?$/.test(x)

diff --git a/package.json b/package.json
@@ -29,7 +29,7 @@
     "chai": "^3.5.0",
     "coveralls": "^2.11.12",
     "mocha": "^3.0.1",
-    "nyc": "^10.0.0",
+    "nyc": "^11.2.1",
     "standard": "^10.0.2",
     "standard-version": "^4.0.0"
   },

diff --git a/test/yargs-parser.js b/test/yargs-parser.js
@@ -1955,6 +1955,17 @@ describe('yargs-parser', function () {
         })
         expect(parsed._[0]).to.equal('5')
       })
+
+      it('parses number if option explicitly set to number type', function () {
+        var parsed = parser(['--foo', '5', '--bar', '6'], {
+          number: 'bar',
+          configuration: {
+            'parse-numbers': false
+          }
+        })
+        expect(parsed['foo']).to.equal('5')
+        expect(parsed['bar']).to.equal(6)
+      })
     })
 
     describe('boolean negation', function () {
@@ -2440,4 +2451,17 @@ describe('yargs-parser', function () {
     })
     argv.a.should.deep.equal(['a.txt', 'b.txt'])
   })
+
+  // see: https://github.com/yargs/yargs/issues/963
+  it('does not magically convert numeric strings larger than Number.MAX_SAFE_INTEGER', () => {
+    const argv = parser([ '--foo', '93940495950949399948393' ])
+    argv.foo.should.equal('93940495950949399948393')
+  })
+
+  it('converts numeric options larger than Number.MAX_SAFE_INTEGER to number', () => {
+    const argv = parser([ '--foo', '93940495950949399948393' ], {
+      number: ['foo']
+    })
+    argv.foo.should.equal(9.39404959509494e+22)
+  })
 })