test: ensure BigInts are decoded as BigInts

[achingbrain]

If you encode an object with a BigInt property, you get a Number back when the buffer is decoded - is this intentional?

Also, the node tests appear to be run during npm test:browser?

This PR doesn't fix anything but does add a failing test as I'm not sure whether this is intended behaviour or not.

[rvagg]

is this intentional

Yes, from the README, we're in this position with integers:

Integers larger than Number.MAX_SAFE_INTEGER or less than Number.MIN_SAFE_INTEGER will be encoded as floats. There is no way to safely determine whether a number has a fractional part outside of this range.

for encode():

BigInts are supported by default within the 64-bit unsigned range but will be also be encoded to their smallest possible representation (so will not round-trip as a BigInt if they are smaller than Number.MAX_SAFE_INTEGER). Larger BigInts require a tag (officially tags 2 and 3).

for decode():

Integers (major 0 and 1) that are outside of the safe integer range will be converted to a BigInt.
...
allowBigInt (boolean, default true): when an integer outside of the safe integer range is encountered, an error will be thrown. To disallow BigInts on encode, a custom type encoder for 'bigint' will need to be supplied.

So, tag-free, cborg does this:

• Freely accept BigInt and Number, interchangably where it makes sense, but where a number can be assumed to be an "integer" (because it's a BigInt or has no fractional component) it gets encoded as the smallest possible form in CBOR (16, 32 or 64 bits)
• Reject any BigInt greater than the 64-bit range, because encoding anything that big will require a tag (2 or 3), although they are offered as optional add-ons for people that want them: https://github.com/rvagg/cborg/blob/master/taglib.js
• On decode, integers get returned as Numbers except when they're outside of the safe integer range, so there's a space between +/- 2^53 and +/- 2^64 where you'll get a BigInt instead of a Number when you have very large integer.

And there are existing tests to probe all of these boundaries too -- if we made this new test pass, a bunch of others would fail.

These decisions are mostly driven around DAG-CBOR obviously, and here's the set of factors there that contribute to the state of play:

• DAG-CBOR doesn't allow any additional tags to properly encode numbers outside of the 64 bit range (floats or ints), if you want to do that then you should use Bytes and have a conversion mechanism -- which is what Filecoin does.
• borc ships with a whole bunch of tags supported, hence the round-tripping of bignumber.js values, because they get encoded as tags, but that only works in JS because no other DAG-CBOR implementation ships with additional tags.
• Because of the lack of tags, and the smallest-possible encoding rule, we have no way to signal that a number should round-trip as something specific. So we have no mechanism to pass "this should be a BigInt" into the encoded form so it'll round-trip properly.
• We're kind of screwed with the integer/float ambiguity in JS, and we're also screwed with the 2^53 problem for integers, so we have to make tradeoffs somewhere.

So the current implementation takes one of the possible paths here to dealing with integers in JavaScript. There are other options we could take, and these are certainly worth ongoing discussions so I wouldn't rule out a shift here if we decide on better API choices:

1. Reject BigInt entirely, and only encode Number objects as integers when Number.isInteger() returns true: this rules out the possibility of integers outside of the +/- 2^53 range and also means that if we get an encoded integer outside of that range it'll be a Number but Number.isInteger() will (probably) return false and it'll essentially be useless to someone trying to do something meaningful with it (i.e. it'll actually be "unsafe"). This is not unreasonable though, we use the word "safe" for a reason and the recommendation for IPLD in general could just be to avoid integers outside of +/- 2^53 and do what Filecoin is doing if you want to support large numbers. Out current codec docs essentially have recommendations along these lines anyway.
2. Make everything a BigInt - either convert incoming Number.isInteger()===true objects to a BigInt or encode them all as floats! This would be a clean technical solution because we have clear delineation of kinds, preserve kind distinction for round-trips and we avoid 2^53 problems entirely. Beautiful! (Although we'd still have to reject BigInts > 64-bit). But kind of user-hostile for JS users - at least that's my current take. Maybe this is worth revisiting as BigInt gains more widespread adoption?
3. Current position - accept BigInt and Number.isInteger()===true objects as integers and encode as smallest-possible, decode then only uses BigInt for integers that are outside of the safe range.

[rvagg]

yeah, this looks like an error!

[vmx]

I think the current way is the most user-friendly one.

From an IPLD perspective, I think it would be great to have a mode, where IPLD Data-Model integers are always BigInt and IPLD Data-Model Floats are always Number. This way JS would map cleanly to the IPLD Data-Model. Though I'm realy not sure how usable this will be when actually used within JS.

[achingbrain]

I think the current way is the most user-friendly one

Ha, you're joking, right? You put one type in, you get another type back and they are completely incompatible, though it's hard to see how it would be any other way without a schema.

Anyway thanks for the detailed explanation @rvagg, the tradeoffs that have been made are clear and this PR should be closed.