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

BigInt should be handled by native codec instead of extension codec #115

Open
bin-y opened this issue Jun 23, 2020 · 8 comments · May be fixed by #211
Open

BigInt should be handled by native codec instead of extension codec #115

bin-y opened this issue Jun 23, 2020 · 8 comments · May be fixed by #211

Comments

@bin-y
Copy link

bin-y commented Jun 23, 2020

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

@gfx
Copy link
Member

gfx commented Jun 23, 2020

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.

@bin-y
Copy link
Author

bin-y commented Jun 23, 2020

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.

@shchemelevev
Copy link

+1

@joshyrobot
Copy link

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.

@arendjr
Copy link

arendjr commented Apr 20, 2022

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.

@jasonpaulos jasonpaulos linked a pull request May 5, 2022 that will close this issue
@jasonpaulos
Copy link

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.

@gfx
Copy link
Member

gfx commented Mar 6, 2023

Thank you for your effort, @jasonpaulos, but I've made another PR for bigint support: #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.

@jasonpaulos
Copy link

I responded in that PR with an issue I noticed: #223 (comment)

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.

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

Successfully merging a pull request may close this issue.

6 participants