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

@@            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              

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

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?