feat: Support BigInt in binary, compact, json protocols#195
feat: Support BigInt in binary, compact, json protocols#195kevin-greene-ck wants to merge 10 commits intomasterfrom
Conversation
|
|
||
| if (isInt64(i64)) { | ||
| if (typeof i64 === 'bigint') { | ||
| assertBigIntRange(i64) |
| public static MAX_64_BIT_INTEGER = 9_223_372_036_854_775_807n | ||
| public static MIN_64_BIT_INTEGER = -9_223_372_036_854_775_808n | ||
|
|
||
| public static assert64BitRange(num: bigint): void { |
There was a problem hiding this comment.
I started by adding this method in utils that created a circlular requrie in the generated JS files that because of the way TypeScript enums are generated resulted in null pointer exceptions.
Storing here seems okay for now. Will revisit if we ever remove Int64 support.
| public readI64(): Int64 { | ||
| public readI64(type?: 'int64'): Int64 | ||
| public readI64(type: 'bigint'): bigint | ||
| public readI64(type?: I64Type): bigint | Int64 { |
There was a problem hiding this comment.
apart from the naming, whats the point of returning a bigint?
There was a problem hiding this comment.
Not sure I understand the question. This gets threaded back to the codegen (and ultimately the app code) and based on user options the application is provided with the object they want BigInt or Int64. So we're reading / creating different objects based on what the user wants. More context if looking at the corresponding calls from the codegen PR.
There was a problem hiding this comment.
it seems weird to me to call readI64 and getting a bigint in return.
i'm also not sure what the usecase is that you would want a bigint, when it'll only even have 64bits of precision
There was a problem hiding this comment.
readI64 here refers purely to the Thrift type we're reading from the Buffer, not the the return type. The goal here is to allow users the option to use a JavaScript primitive value that will satisfy the needs of storing an i64 without having to rely on an external library. It is difficult to do anything useful with an Int64 whereas a BigInt just works with all the usual JS operators. Also, if we were to eventually drop Int64 support the Thrift libraries could be simplified as Buffer already has methods for working with BigInt instead of having to work with the upper and lower bits separately and merging as is now required.
| } | ||
|
|
||
| public toBigInt(): bigint { | ||
| return this.buffer.readBigInt64BE() |
There was a problem hiding this comment.
is the return type buffer or bigint?
There was a problem hiding this comment.
The return type is bigint. The readBigInt* methods on Buffer return a bigint reading from Buffer.
kevin-greene-ck
requested review from
danielchencreditkarma,
danielle-illing-ck and
ramakrishna-battala-ck
4 participants
Summary
Context
When this library was started the only way to correctly represent 64-bit integers in JavaScript was with
Int64(or another similar library). Because Apache Thrift usesInt64that is what we stuck with.At this point JavaScript has supported large integers via
BigIntfor a while.Changes
i64values stored asBigInt.Int64to work withBigInt. This includes methods to create aBigIntfrom anInt64and create anInt64from aBigInt.These changes are optimized toward keeping old code paths unchanged. Changes are still biased toward using
Int64. In a later breaking release this will change toward prioritizingBigIntand perhaps removingInt64.