sidorares / json-bigint

JSON.parse/stringify with bigints support
MIT License
790 stars 189 forks source link

potential bug on large numeric values #86

Open jcoveney-anchorzero opened 1 year ago

jcoveney-anchorzero commented 1 year ago

hello! I'd be happy to file a fix for this, but I wanted to understand the intent behind the code. I imagine that for someone who knows the code well it'd be pretty easy? (assuming it's something that y'all want to fix)

regardless, the issue starts here

https://github.com/sidorares/json-bigint/blob/master/lib/parse.js#L204

in my case, string = '123456789012345678901234567890.012345678901234567890123456789' since number = +string, then number = 1.2345678901234568e+29.

as Number.isSafeInteger(number) is false, we get to

          return _options.storeAsString
            ? string
            : /[\.eE]/.test(string)
            ? number
            : _options.useNativeBigInt
            ? BigInt(string)
            : new BigNumber(string);

/[\.eE]/.test(string) is true, so this ends up just returning number, so 1.2345678901234568e+29, leading to a loss of precision I'd like to avoid.

I'm wonder if the intent might simply that this library wants to handle bigint values, but doesn't do anything specifically to handle large numeric values? in which case perhaps a PR wouldn't be welcome...but I'd be happy to extend it to support large numeric values. give BigNumber is already being used, it seems like it wouldn't be hard? could also add an option.

jcoveney-anchorzero commented 1 year ago

a sort of hacky approach that I think could be improved on, but I think works?

      number = +string;
      if (!isFinite(number)) {
        error('Bad number');
      } else {
        if (BigNumber == null) BigNumber = require('bignumber.js');
        if (Number.isSafeInteger(number))
          return !_options.alwaysParseAsBig
            ? number
            : _options.useNativeBigInt
            ? BigInt(number)
            : new BigNumber(number);
        else
          // Number with fractional part should be treated as number(double) including big integers in scientific notation, i.e 1.79e+308
          return _options.storeAsString
            ? string
            : /[\.eE]/.test(string)
            ? (new BigNumber(string).isEqualTo(new BigNumber(number)) ? number : new BigNumber(string))
            : _options.useNativeBigInt
            ? BigInt(string)
            : new BigNumber(string);
      }
    },

(there has gotta be a better way to do this than new BigNumber(string).isEqualTo(new BigNumber(number)), but I don't know typescript/javascript well enough)

victorkirov commented 4 months ago

Would /[\.eE]/.test(string) not be false? The string value doesn't look like it has an exponential in it 👀