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

potential bug on large numeric values #86

Open
jcoveney-anchorzero opened this issue Jun 22, 2023 · 2 comments
Open

potential bug on large numeric values #86

jcoveney-anchorzero opened this issue Jun 22, 2023 · 2 comments

Comments

@jcoveney-anchorzero
Copy link

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
Copy link
Author

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants