msgpack / msgpack-javascript

@msgpack/msgpack - MessagePack for JavaScript / msgpack.org[JavaScript/TypeScript/ECMA-262]
https://msgpack.org/
ISC License
1.3k stars 162 forks source link

optional BigInt support: map BigInt to int64/uint64 when `useBigInt64` is set to true #223

Closed gfx closed 1 year ago

gfx commented 1 year ago

This PR introduces a new option useBigInt64: boolean, which enables "bigint mode" in this library.

Unfortunately, the bigint-mode handles JavaScript numbers completely differently than non-bigint-mode. The following table describes the type mappings.

useBigInt64: trueuseBigInt64: false
a JavaScript safe integer but larger than UINT32_MAXmsgpack float64msgpack uint64
a JavaScript safe integer but smaller than INT32_MINmsgpack float64msgpack int64
a JavaScript bigintmsgpack int64/uint64N/A or extension

Because JavaScript's bigint is not calculable with number, they should not be mixed. A field that is a bigint must always be a bigint even if it's encoded and decoded by this library.

The default value of the option will be changed to true in a future major upgrade. Not sure when I'll do it so far.

ref. #115

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06 :tada:

Comparison is base (daae51c) 98.31% compared to head (19ff295) 98.38%.

:exclamation: Current head 19ff295 differs from pull request most recent head df46c60. Consider uploading reports for the commit df46c60 to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #223 +/- ## ========================================== + Coverage 98.31% 98.38% +0.06% ========================================== Files 16 16 Lines 952 990 +38 Branches 193 202 +9 ========================================== + Hits 936 974 +38 Misses 16 16 ``` | [Impacted Files](https://codecov.io/gh/msgpack/msgpack-javascript/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=msgpack) | Coverage Δ | | |---|---|---| | [src/decode.ts](https://codecov.io/gh/msgpack/msgpack-javascript/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=msgpack#diff-c3JjL2RlY29kZS50cw==) | `100.00% <ø> (ø)` | | | [src/decodeAsync.ts](https://codecov.io/gh/msgpack/msgpack-javascript/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=msgpack#diff-c3JjL2RlY29kZUFzeW5jLnRz) | `90.47% <ø> (ø)` | | | [src/encode.ts](https://codecov.io/gh/msgpack/msgpack-javascript/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=msgpack#diff-c3JjL2VuY29kZS50cw==) | `100.00% <ø> (ø)` | | | [src/Decoder.ts](https://codecov.io/gh/msgpack/msgpack-javascript/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=msgpack#diff-c3JjL0RlY29kZXIudHM=) | `99.19% <100.00%> (+0.02%)` | :arrow_up: | | [src/Encoder.ts](https://codecov.io/gh/msgpack/msgpack-javascript/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=msgpack#diff-c3JjL0VuY29kZXIudHM=) | `98.13% <100.00%> (+0.19%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=msgpack). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=msgpack)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jasonpaulos commented 1 year ago

Hi @gfx, thank you for working on this feature. For the most part it looks good but I wanted to point out one design issue.

I see that you made the deliberate choice to encode and decode bigints completely separately from other integers. As a result, this code will always encode a JavaScript bigint as a msgpack uint64 or int64, even if it is representable by a 32-bit integer or smaller.

For example, if I encode BigInt(1) with this code it would produce the following byte array: [207, 0, 0, 0, 0, 0, 0, 0, 1]. However I think many people will expect this to produce [1] instead, i.e. the same encoding that the JavaScript number 1 would produce.

I say this because it's common for JavaScript msgpack code to be used in conjunction with components written in other languages, which don't have the same number vs bigint notion as JavaScript does. The msgpack implementation in these other languages would expect an integer to be encoded using the smallest possible representation, which right now isn't the case.

I understand your reluctance to enable this behavior by default, since as you pointed out bigints and numbers should not be used together in arithmetic. However, would you be willing to creating an additional option to make this behavior possible?