Ignore numeric-looking strings with huge values

[shadowspawn]

An issue people encounter using minimist (as reported on old repository) is that false-positive numeric-looking strings are unexpectedly converted to numbers with data loss, whether losing precision or being converted to infinity. For example, values for twitter or YouTube ids.

The README says:

Numeric-looking arguments will be returned as numbers unless opts.string or opts.boolean is set for that argument name.

The suggested author work-around of explicitly setting the option parsing does avoid the automatic number conversion for options, but this approach is not available for positional arguments.

A possible improvement is to follow the lead of yargs and not attempt to coerce numeric-looking strings which are too big to reasonably represent: yargs/yargs-parser#110

(This only addresses one issue with automatic numeric conversion, and I don't mind if we leave it out and keep it simple! I had seen the issue reported and wanted to look into the problem, and how the code had evolved in Yargs which I knew had added some extra checks.)

[codecov-commenter]

Codecov Report

Merging #18 (abca18e) into main (5784b17) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #18   +/-   ##
=======================================
  Coverage   98.75%   98.76%           
=======================================
  Files           1        1           
  Lines         161      162    +1     
  Branches       71       72    +1     
=======================================
+ Hits          159      160    +1     
  Misses          2        2           

Impacted Files	Coverage Δ
index.js	98.76% <100.00%> (+<0.01%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[shadowspawn]

isSafeInteger was introduced with Node.js 0.12. If there is interest in this PR, can probably hand-code an equivalent which runs in older versions.

[ljharb]

isSafeInteger isn’t available in all the engines we support.

[shadowspawn]

Yes. I looked up when isSafeInteger was introduced but then forgot to check the oldest supported version for Minimist, or lack of an explicit minimum. Realised when I saw the test fail.

The PR isn't a no-brainer so I'll only try and make it work with older nodes if otherwise of interest?

[ljharb]

Suggested change

	if (!Number.isSafeInteger(Math.floor(x))) { return false; }
	if (x >= Math.pow(2, 53)) { return false; }

i think this would do it?

My concern is whether this bugfix could arguably be considered a breaking change, in which case it wouldn't likely be worth it.

[shadowspawn]

Yes, being conservative it is arguably a breaking change, and it isn't a full solution.

[shadowspawn]

I wanted to look into it, and have! Have some clues for future if needed. Not compelling enough for now.

[shadowspawn]

For interest, I found a copy of the original reported Minimist bug: meszaros-lajos-gyorgy/minimist-lite#2

[shadowspawn]

The suggested author work-around of explicitly setting the option parsing does avoid the automatic number conversion for options, but this approach is not available for positional arguments.

Actually, can specify positional are "string" by specifying _: #52.

[shadowspawn]

I noticed this is touched on in the Open Group Base Specifications for Utility Conventions

6. Unless otherwise specified, whenever an operand or option-argument is, or contains, a numeric value:

• The number is interpreted as a decimal integer.
• Numerals in the range 0 to 2147483647 are syntactically recognized as numeric values.
  ...

So there is some precedent for limiting the numbers, if we choose to in the future.