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

Update README.md #168

Merged
merged 2 commits into from
Apr 1, 2021
Merged

Update README.md #168

merged 2 commits into from
Apr 1, 2021

Conversation

Kapsonfire-DE
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-DE
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-DE
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-DE
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