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

Handling (de)serialization modes for 64-bit integer types #10

Closed
scvnc opened this issue Feb 2, 2018 · 5 comments · Fixed by #37
Closed

Handling (de)serialization modes for 64-bit integer types #10

scvnc opened this issue Feb 2, 2018 · 5 comments · Fixed by #37

Comments

@scvnc
Copy link

scvnc commented Feb 2, 2018

If I add the [jstype=JS_STRING] option to a message then the generated stubs will return a string for the 64-bit integer value. The generated typescript definition does not handle this. I'll let you know if I attempt a pull request to add this functionality but for now I am overriding the typescript guards in my code for now.

Example

message Book {
    int64 isbn = 1 [jstype=JS_STRING];
    string title = 2;
    string author = 3;
}

Note that the generated code has a readInt64String() invocation that works as expected. The value of the message is of type string instead of number.

proto.core.Book.deserializeBinaryFromReader = function(msg, reader) {

  // ... 
  var value = /** @type {string} */ (reader.readInt64String());
  msg.setTarget(value)

}

Background

protocolbuffers/protobuf#3666

When taking the static stub generation google/protobuf has an obscure configuration that will (de)serialize 64-bit integers into strings. I am satisfied with parsing this with some other module that can manipulate the long number at this point.

@agreatfool
Copy link
Owner

Thank you for your issue. This surely is the issue shall be fixed.
And these days I found 3rd lib protobuf.js already has the functionality to handle gRPC js code generation & d.ts definition. You can have a look.
From version 6.8.0.

@scvnc
Copy link
Author

scvnc commented Feb 5, 2018

Thanks for making this nice tool.

Yeah, protobuf.js seems to be used by grpc-node when dynamically generating the code at runtime. It does handle serialization into Long.js which is very nice. Since we want TypeScript support: having the code statically generated with typedefs makes sense. I don't have a clear recipe for combining protobuf.js with statically generated client stubs/messages but would be interested to know if you have accomplished that.

@agreatfool
Copy link
Owner

agreatfool commented Feb 6, 2018

I just found in the doc of version 6.8.4, protobuf.js provided the functionality to generate static code files.

Please check here: Command Line > pbjs for JavaScript

$> pbjs -t static-module -w commonjs -o compiled.js file1.proto file2.proto
will generate static code for definitions within file1.proto and file2.proto to a CommonJS module compiled.js.

And also: Command Line > pbts for TypeScript

$> pbts -o compiled.d.ts compiled.js

I just made some test yesterday, seems it works very good. Maybe you can have a try.

The tool grpc-tools used to generate javascript code from protobuf provided by google official, just, how to say, "javascript version too old". Which just not following the latest javascript grammar & javascript development trends.

So I think maybe protobuf.js is a better choice for the users who want static code generation, just like you and me.

@agreatfool
Copy link
Owner

One more issue have to be fixed before using protobuf.js as a grpc static code generator.

Since protobuf.js is a tool protobuf oriented, not grpc oriented. So it provides the option which rpc tool you want to use. That means the code generated with protobuf.js has no rpc implementation. Not like grpc-tools, that tool would generate code with gRPC official implementation.

See Examples > Using services and examples/streaming-rpc.js.

And I searched around, seems no official or 3rd tutorial how to combine protobuf.js with gRPC. It may cost a lot of time to solve this issue.

@scvnc
Copy link
Author

scvnc commented Feb 8, 2018

Good to resonate on that clarity. Even if protobuf.js does not handle the grpc service implementation it may be worthwhile. Later, perhaps there could be some way to decouple the grpc service generation from this project.

sangaman added a commit to ExchangeUnion/xud that referenced this issue Dec 13, 2018
This adds `jstype = JS_STRING` annotations on the channel ids in the
lnd proto definition so that they use `string` types instead of numbers.
Javascript does not natively support 64 bit numbers and precision
can be lost, and the channel ids are known to cause issues because of
this.

This also adds a new package to generate type definitions from the
`.proto` files. This is because of a known bug in the previously used
plugin, agreatfool/grpc_tools_node_protoc_ts#10, which does not update
the generated type definition when `jstype = JS_STRING` is used.

Fixes #745.
sangaman added a commit to ExchangeUnion/xud that referenced this issue Dec 17, 2018
This adds `jstype = JS_STRING` annotations on the channel ids in the
lnd proto definition so that they use `string` types instead of numbers.
Javascript does not natively support 64 bit numbers and precision
can be lost, and the channel ids are known to cause issues because of
this.

This also adds a new package to generate type definitions from the
`.proto` files. This is because of a known bug in the previously used
plugin, agreatfool/grpc_tools_node_protoc_ts#10, which does not update
the generated type definition when `jstype = JS_STRING` is used.

Fixes #745.
sangaman added a commit to ExchangeUnion/xud that referenced this issue Dec 18, 2018
This adds `jstype = JS_STRING` annotations on the channel ids in the
lnd proto definition so that they use `string` types instead of numbers.
Javascript does not natively support 64 bit numbers and precision
can be lost, and the channel ids are known to cause issues because of
this.

This also adds a new package to generate type definitions from the
`.proto` files. This is because of a known bug in the previously used
plugin, agreatfool/grpc_tools_node_protoc_ts#10, which does not update
the generated type definition when `jstype = JS_STRING` is used.

Fixes #745.
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 a pull request may close this issue.

2 participants