Skip to content

Conversation

@Kapsonfire
Copy link
Contributor

Possible to save size in BigInt Handling, when using Number if its in SAFE range

Possible to save size in BigInt Handling, when using Number if its in SAFE range
@gfx
Copy link
Member

gfx commented Mar 30, 2021

Thanks for your suggestion, but I'm not sure it's a good idea. Rather, I suspect it introduces unnecessary complexity because JavaScript's BigInt is not compatible with the primitive number. There might be cases where that trick is useful, but I believe we shouldn't do that in general.

@Kapsonfire
Copy link
Contributor Author

Thats why it checks for the SAFE Representations. These are always safe to work with

@gfx
Copy link
Member

gfx commented Mar 31, 2021

I said "not compatible" because you cannot mix BigInt and Number in arithmetic operations:

const x = BigInt(1) + 1; // => TypeError: Cannot mix BigInt and other types, use explicit conversions

So if you decode a MessagePack binary in this way, you must check the type of values that might be a BigInt. On the other hand, if a particular field is always a BigInt, you don't need to check its type.

@Kapsonfire
Copy link
Contributor Author

But its always BigInt. The Decoder always returns BigInt, even if its encoded as number and not as string.
Your argument is only valid, if the Decoding side doesnt decode with the same CodecExtension. But thats true for every CodecExtension

@gfx
Copy link
Member

gfx commented Apr 1, 2021

Oh, I'm sorry! I misunderstood this PR. This PR changes only how BigInt is encoded in order to optimize the encoded binary!

Okay, so it looks good to me. Could you update codec-bigint.test.ts as well?

Changed the test to encode as number if its in SAFE_INTEGER Area.
@Kapsonfire
Copy link
Contributor Author

Done :-)

@gfx
Copy link
Member

gfx commented Apr 1, 2021

Thank you for your contribution! Will merge it after CI passes.

@gfx gfx merged commit 729ad6d into msgpack:main Apr 1, 2021
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.

2 participants