fix: Throw on BigInt type values #397
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reggi
approved these changes
Sep 17, 2020
nbbeeken
commented
Sep 17, 2020
mbroadst
suggested changes
Sep 18, 2020
mbroadst
approved these changes
Sep 18, 2020
src/parser/serializer.ts
Outdated
| @@ -806,6 +806,8 @@ export function serializeInto( | |||
| index = serializeString(buffer, key, value, index, true); | |||
| } else if (typeof value === 'number') { | |||
| index = serializeNumber(buffer, key, value, index, true); | |||
| } else if (typeof value === 'bigint') { | |||
| throw new TypeError('Do not know how to serialize a BigInt, please use Decimal128'); | |||
There was a problem hiding this comment.
Suggested change
| throw new TypeError('Do not know how to serialize a BigInt, please use Decimal128'); | |
| throw new TypeError('Unsupported type BigInt, please use Decimal128'); |
this is just a deep nitpick, I think we do know how to serialize them, we just don't support them being passed in raw for now
There was a problem hiding this comment.
It just matches JSON's error precisely
But I think I agree and I'll change this
Given JavaScript's new BigInt type the serializer will attempt to fit the value in the smallest possible type. In the following order: Int32, Int64, Decimal128. Any values larger than Decimal128's maximum representable range will result in a thrown TypeError NODE-2529
In order to ensure users get consistent and expected behavoir BigInts should be manually translated to a type which can fit the precision of the value
nbbeeken
force-pushed
the
NODE-2529/bigint
branch
from
823f711
to
f6a51b2
Compare
emadum
approved these changes
Sep 21, 2020
nbbeeken
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Given JavaScript's new BigInt type the serializer will attempt to fit the value in the smallest possible type. In the following order: Int32, Int64, Decimal128. Any values larger than Decimal128's maximum representable range will result in a thrown TypeError
NODE-2529
There's no logic in here to deserialize to big int only to accept it and serialize it to one of our BSON types. In terms of platform support I only used the BigInt constructor not the
23nsyntax so the code path will just go untravelled on older JS engines.