Translating int64 to TypeScript Number type sacrifices accuracy

[johanbrandhorst]

The TypeScript Number type is a floating point number, which means attempting to express the int64 range in numbers will lead to inaccuracies. The protoc compiler therefore uses strings when expressing large numeric types in JSON.

Perhaps something similar should be considered for the TypeScript type translation?

[jonnyreeves]

The protoc compiler therefore uses strings when expressing large numeric types in JSON.

I was under the impression that protoc only ever used javascript's Number type to express int64 values. protocolbuffers/protobuf#2214 looks to address this but has not landed.

[johanbrandhorst]

I'm talking specifically about the JSON representation of int64 integers. The JS library may well be using the Number type, so this might be impossible to remedy here, but I thought it'd be good to have this discussion anyway.

[ghost]

Hi @johanbrandhorst I've looked into this in the past and currently there is no official support in the public version of protobuf to use strings to preserve precision. There are a couple of solutions in progress though:

• There's this PR which would fix this bug in the JS implementation: JavaScript: Use strings for 64-bit ints to preserve precision protocolbuffers/protobuf#1832 (which if landed would mean ts-protoc-gen should be updated to accept string | number in int64 setters)
• There's also the field annotation google.protobuf.FieldOptions.JSType which lets you choose per field whether it should use strings or numbers (unfortunately there's no plan to make string the default representation).

Both of these would fix the underlying problem that, although you can use the field setters with a string, just before they're serialised the values are converted to numbers and so precision is lost. There are functions to serialise string encoded int64s but these aren't used in the public version of protobuf (the Google internal version support the JSType annotation and if set to string uses writeInt64String instead of writeInt64)

Although it's not used in generated code, the writeInt64String method is exposed in the protobuf library so there is a hack to make this work:

import { BinaryWriter, BinaryReader } from 'google-protobuf';

Object.defineProperty(BinaryWriter.prototype, 'writeInt64', {
  value: function(field: number, value: number | string) {
    return BinaryWriter.prototype.writeInt64String.call(this, field, value.toString());
  },
});
Object.defineProperty(BinaryReader.prototype, 'readInt64', { value: BinaryReader.prototype.readInt64String });

This is ugly (and doesn't work with the ts-protoc-gen generated types without using something like pb.setMyInt64Field('1309150421589' as any)) but does mean that values are correctly serialised and deserialised as strings

[johanbrandhorst]

@improbable-bradley that's a great writeup, I'll reference this in the future, thanks!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[johanbrandhorst]

protoc has supported this since 3.4.0: https://github.com/google/protobuf/releases/tag/v3.4.0

[jonnyreeves]

Thanks for pointing this out! I've not dived deep on proto field options yet, would you mind providing me with an example proto so I can test this feature, please? Thank you

…

On Mon, 21 May 2018, 14:25 Johan Brandhorst, ***@***.***> wrote: protoc has supported this since 3.4.0: https://github.com/google/protobuf/releases/tag/v3.4.0 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMN-byPzmHlqH0M17mkEAxdXrGCbexrks5t0sBjgaJpZM4Ot2-N> .

[johanbrandhorst]

From protocolbuffers/protobuf#3666 (comment):

message dummy {
    uint64 bigInt = 1 [jstype = JS_STRING];
}

It makes use of the various FromString methods: https://github.com/google/protobuf/blob/dd2dc0f14f9e5dc4e8343cc5f78d5f0bddd8991c/js/binary/arith.js#L289, https://github.com/google/protobuf/blob/dd2dc0f14f9e5dc4e8343cc5f78d5f0bddd8991c/js/binary/encoder.js#L356 etc. I implemented support for this in my GopherJS gRPC-Web library recently: johanbrandhorst/protobuf@7e36e40.

[jonnyreeves]

Thanks @johanbrandhorst; it would be good to add a test-case which uses this field option and provide a mention to it in the README at which point this issue can be closed :)

cc @MarcusLongmuir

[jonnyreeves]

Thanks to @easyCZ's efforts in #80 we are now consistently using protoc v3.5.1 so we should be in a position to test and document this.

[cmoad]

We use the jstype = JS_STRING field annotation extensively and are currently working to translate a project from flow to typescript. I forked the protoc-gen-flow project to simply output string instead of number for these cases and it worked fine. The generated JS will use the string variants of the conversion functions, so the *.d.ts files will actually be correct when they say string.

Here is a link of the changes required for the flow project. We'll be attempting the same for this project.
steckel/protoc-gen-flow@master...andcostello:master