creditkarma / thrift-typescript

Generate TypeScript from Thrift IDL files
Apache License 2.0
155 stars 32 forks source link

Incorrect Int64 constants #137

Open mustafa-cosar opened 5 years ago

mustafa-cosar commented 5 years ago

The int64 constants are being generated incorrectly.

const i64 BIG_INTEGER = 9223372036854775807

is being emitted as

export const BIG_INTEGER: Int64 = new Int64(9223372036854776000);

As you can see there is a loss of information.

The correct output should be like the following, using string constructor:

export const BIG_INTEGER: Int64 = new Int64('9223372036854775807');

===========================

Edit: The above constructor argument is wrong, the correct one should be:

export const BIG_INTEGER: Int64 = new Int64('7FFFFFFFFFFFFFFF');

Thanks, @kevin-greene-ck for pointing it out.

kevin-greene-ck commented 5 years ago

This isn't that simple of a fix. The Int64 constructor does not except a string in this form. If you pass a string it assumes the string is hex-encoded (https://github.com/broofa/node-int64/blob/master/Int64.js#L49). In the apache library there is a util for creating an Int64 from a string (https://github.com/apache/thrift/blob/master/lib/nodejs/lib/thrift/int64_util.js#L61), but this isn't publicly exported. We solve this for thrift-server by subclassing Int64 and adding this functionality (https://github.com/creditkarma/thrift-typescript/blob/master/src/main/render/thrift-server/values.ts#L79). The behavior we're going with is consistent with the official apache code generator which allows for loss of precision (https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/generate/t_js_generator.cc#L596). I would like to fix this, but I'm not sure when the core team will have a chance to get to it.