msgpack-javascript icon indicating copy to clipboard operation
msgpack-javascript copied to clipboard

BigInt should be handled by native codec instead of extension codec

Open bin-y opened this issue 4 years ago • 8 comments

It is not a good choice to handle 64bit integers with extension codec. While 64 bit integers are built-in types of msgpack and BigInt is a built-in type of javascript, solving 64 bit problem with extension codec is not an compatible way when making cross-language systems. If javascript developers use an extension codec to support 64bit integers, then developers in other languages will have to write an corresponding extension codec, while msgpack implementation in other languages (c++, c, python, java, etc.) natively support mapping 64 bit integers into their language-specific type.

To support BigInt, I think this library should map 64 bit integers to BigInt natively or this library should add support to register custom codec for built-in types.

original issue #114

bin-y avatar Jun 23 '20 05:06 bin-y

BigInt is not compatible with numbers (i.e. 64-bit floating-point numbers) in JavaScript, and BigInt is not compatible with 64-bit ints in general, so I suppose implicit mapping of BigInt and 64-bit ints should cause problems.

However, if one knows what they are doing, its mapping could be helpful for cross-platform cooperation, so I'd like to consider an optional feature to map BigInt and 64-bit ints.

gfx avatar Jun 23 '20 05:06 gfx

Glad to hear that you have take mapping BigInt and 64-bit integers into consideration. But I don't think it is optional. Yes, BigInt is not compatile with javascript Number as Number in javascript is something like double, but no, BigInt should be compatible with 64-bit integers of msgpack as integers and floats are different things in general and as the existence of BigInt.asIntN and BigInt64Array.

bin-y avatar Jun 23 '20 06:06 bin-y

+1

shchemelevev avatar Sep 10 '20 07:09 shchemelevev

BigInt is not compatible with 64-bit ints

What? Of course it is, we even have wonderful methods on DataViews like .getBigUint64(). This is the only proper way to support 64 bit integers, any other method is going to be far less compatible with plain numbers.

joshyrobot avatar Jun 11 '21 08:06 joshyrobot

I also believe adding native BigInt support to handle MessagePack's native 64 bit integers is the right way to go. I also see @gfx's concern, because MessagePack tends to pick the smallest representation when encoding numbers, which could lead to an annoying schism on the JS side.

For example, if I have an u64 in Rust that should be encoded with MessagePack, MessagePack will happily encode it as a 7-bit integer if the value fits, which would come out as a number on the JS side. But if the value were larger than 2^32 - 1 it would use the 64-bit encoding, which would suddenly come out as a BigInt on the JS side. This can be annoying if you have to deal with both (incompatible) types on the JS side.

The current "solution" at least supports number up to 2^53 - 1 consistently on the JS side, but it's not a solution at all for anyone who needs integers bigger than that. So I agree having an optional feature to opt-in to the use of BigInt would be the right way to go that I believe would satisfy everyone.

arendjr avatar Apr 20 '22 08:04 arendjr

I've just made #211, which is a PR to add BigInt support in an opt-in and flexible way. Feedback/support would be appreciated.

jasonpaulos avatar May 05 '22 00:05 jasonpaulos

Thank you for your effort, @jasonpaulos, but I've made another PR for bigint support: https://github.com/msgpack/msgpack-javascript/pull/223 , which introduces an option useBigInt64. It's not so much flexible. It introduces a new mapping for JavaScript's bigint and MessagePack's int64/uint64, as JavaScript's DataView does (it uses DataView's native features, indeed).

I'll release it at the end of March 2023 if nobody has objections.

gfx avatar Mar 06 '23 08:03 gfx

I responded in that PR with an issue I noticed: https://github.com/msgpack/msgpack-javascript/pull/223#issuecomment-1484111129

We are currently using an old fork of this library with bigint support. I would very much like to stop using that fork and use the original library again, but the issue I pointed out in my comment would prevent us from migrating.

jasonpaulos avatar Apr 13 '23 17:04 jasonpaulos