#25 parse very big numeric values

[SebastianG77]

1. Exchanged if (!isFinite(number)) by if (!bigNumberValue.isFinite()) and had to move this function a few lines down in this context, but I think this check is not necessary at all, because whenever a numeric value will be entered, it must be finite anyway.

2. Replaced if (string.length > 15) by if (bigNumberValue.toFixed().length > 15) to ensure big numbers written in scientific notation will also be parsed to BigNumber objects.

3. Added a test case for evaluating my changes.

[SebastianG77]

Hi,

do you already have some feedback regarding this pull request? Will appreciate if you can merge my changes soon as I need this feature now.

[SebastianG77]

Hi,

I just wanted to let you know that I published my fork at npm (https://www.npmjs.com/package/true-json-bigint) as I now really need these changes. Of course, I also referenced your project in the README.md. I hope you don't mind. If I find some time I maybe will keep on extending it by i.e. adding some more test cases etc. It still will be nice if you find some time to merge my pull request.

[sidorares]

Hi @SebastianG77 sorry it's been unanswered for so long. Happy to see you change here. Would you be able to rebase?

[SebastianG77]

Hi,

yes, I generally can do so. Meanwhile you did quiet a lot of changes. Since I am currently quiet busy, I will not be able to have a look at it the next days. So give me a little while until I will have time for it.

However, meanwhile I also merged a pull request of @xamgore and also did some minor changes regarding CI, dependency versions and refactoring in my fork. I also adapted the documentations, but I of course would prefer yours after merging. See my commit history for further details: https://github.com/SebastianG77/true-json-bigint/commits/master. Would be glad to also merge these changes into your repository so there will just be one source to maintain.

[SebastianG77]

Hi @sidorares,

I finally have just updated my PR. Since my changes are almost two years old now, a lot has changed and I had to be very careful with merging your changes. I still think everything is working fine now, but please also have a detailed look on this PR. It also considers all my other changes and I will be happy if you are able to integrate them too.

So in addition to the main changes of my first post this PR also considers the following ones:

1. Added TypeScript support as already announced and defined by @xamgore
2. Test Case "Should show that the 'strict' option will fail-fast on duplicate keys" is now also testing if the correct Exception will be thrown but not just the Exception message. When I updated chai for my fork I came up with this solution and I think it is a little bit more better as more concrete. Hence, I kept my approach.
3. Update test script in package.json, such that mocha will be called by "mocha" but not via "./node_modules/mocha/bin/mocha". Otherwise I always received an error indicating the command "mocha" could not be found.
4. Some unused dependencies have been removed from the test files.
5. Update dependency to mocha 8.1.1 just because I am already considering the latest version in my branch and this would probably be updated anyway in the near future.

[SebastianG77]

I forgot to mention: While testing the merged version, I figured out it is not possible to use scientific notation while having option "useNativeBigInt: true" activated. In this case an error will be thrown now. In your previous versions this issue has been veiled as values written in scientific notation have usually always been handled as ordinary numbers but not as big numbers. However, when activating both options "useNativeBigInt: true" and "alwaysParseAsBig: true" this problem also already occurs in version 1.0.0. I do not think this issue can be fixed here as it obviously directly relates to BigInt, but I still wanted to let you know so you will not be scared if you also bump into the same problem.

[sidorares]

I figured out it is not possible to use scientific notation while having option "useNativeBigInt: true" activated.

can you post small example illustrating this?

[SebastianG77]

Sure, but I have realized I was a bit too overhasty and figured out setting "alwaysParseAsBig: true" will have no effect in this context.

However, you still can construct this this issue with version 1.0.0, but you then need to avoid the "Bad Number" exceptions.

So this example will work as the value of "bigNumber" has 15 characters:

const JSONbig = require('json-bigint')({ useNativeBigInt: true });

const parseResult = JSONbig.parse('{"smallNumber": 1, "bigNumber": 0e1234567890123}')

console.log(parseResult)

This example will not work as "bigNumber" has more than 15 characters, such that the BigInt Object will be created with a string value, but not with a numeric one.

const JSONbig = require('json-bigint')({ useNativeBigInt: true });

const parseResult = JSONbig.parse('{"smallNumber": 1, "bigNumber": 1234567890123e1}')

console.log(parseResult)

On the other hand this example would also work, as here we are using bignumber.js Objects.

const JSONbig = require('json-bigint')({ useNativeBigInt: false });

const parseResult = JSONbig.parse('{"smallNumber": 1, "bigNumber": 12345678901234e1}')

console.log(parseResult)

Anyway, while building a suitable example, I figured out that with this PR the following example will work:

const JSONbig = require('json-bigint')

const parseResult = JSONbig.parse('{"smallNumber": 1, "bigNumber": 3e+500}')

But this one will still return a "Bad Number" Exception:

const JSONbig = require('json-bigint')

const parseResult = JSONbig.parse('{"smallNumber": 1, "bigNumber": 1e12345678}')

The reason for that is that I replaced function Number.isFinite() by BigNumber.isFinite(). According to the API-documention, this function should only return false if values NaN, Infinity or -Infinity are checked (https://mikemcl.github.io/bignumber.js/#isF). However, obviously this function also returns Infinity if one uses very big BigNumber-Objects like 1e12345678.

I would still like to leave it as it is for now, because even though we still will get some undesired "Bad Number" Exception, this PR reduces the cases that will lead to such an exception.

[SebastianG77]

It is a bit late now, but I have fixed the example in my previous comment as the original one did not illustrate the problem I wanted to show you.