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

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

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

gfx
Copy link
Member

@gfx gfx commented Mar 4, 2023

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
Copy link

Codecov Report

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

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

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

📣 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
@@            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 Coverage Δ
src/decode.ts 100.00% <ø> (ø)
src/decodeAsync.ts 90.47% <ø> (ø)
src/encode.ts 100.00% <ø> (ø)
src/Decoder.ts 99.19% <100.00%> (+0.02%) ⬆️
src/Encoder.ts 98.13% <100.00%> (+0.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@gfx gfx merged commit ec05e2e into main Mar 6, 2023
@gfx gfx deleted the gfx/bigint branch March 6, 2023 08:15
@jasonpaulos
Copy link

jasonpaulos commented Mar 26, 2023

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?

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 this pull request may close these issues.

3 participants