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

Invalid code generation for grpc-js Timestamp conversion #754

Closed
aokht opened this issue Jan 5, 2023 · 1 comment · Fixed by #755
Closed

Invalid code generation for grpc-js Timestamp conversion #754

aokht opened this issue Jan 5, 2023 · 1 comment · Fixed by #755
Labels

Comments

@aokht
Copy link
Contributor

aokht commented Jan 5, 2023

Following proto schema

  rpc GetTime(google.protobuf.Empty) returns (google.protobuf.Timestamp);

generates following client code with outputServices=grpc-js

  getTime(request: Empty, callback: (error: ServiceError | null, response: Date) => void): ClientUnaryCall;

But actual response type is Timestamp, not Date

    client.getTime({}, (error, response) => {
      console.log(response) // { seconds: 1672938038, nanos: 388586128 }
    })

It seems the generated service code for deserialization is incorrect.

  getTime: {
    path: "/namespace.Service/GetTime",
    requestStream: false,
    responseStream: false,
    requestSerialize: (value: Empty) => Buffer.from(Empty.encode(value).finish()),
    requestDeserialize: (value: Buffer) => Empty.decode(value),
    responseSerialize: (value: Date) => Buffer.from(Timestamp.encode(toTimestamp(value)).finish()),
    responseDeserialize: (value: Buffer) => Timestamp.decode(value),  // <- should be wrapped by fromTimestamp()
  },

Also useDate=false doesn't generate valid code for serialization.

  • true
    responseSerialize: (value: Date) => Buffer.from(Timestamp.encode(toTimestamp(value)).finish()),  // correct
  • string
    responseSerialize: (value: string) => Buffer.from(Timestamp.encode(toTimestamp(value)).finish()),  // correct
  • false
    responseSerialize: (value: Timestamp) => Buffer.from(Timestamp.encode(toTimestamp(value)).finish()),  // compile error: shouldn't be wrapped by toTimestamp()

I'll open a PR to fix them.

aokht added a commit to aokht/ts-proto that referenced this issue Jan 5, 2023
stephenh pushed a commit that referenced this issue Jan 7, 2023
* fix: #754 timestamp conversion for grpc-js

* test: timestamp conversion for grpc-js
stephenh pushed a commit that referenced this issue Jan 7, 2023
## [1.137.1](v1.137.0...v1.137.1) (2023-01-07)

### Bug Fixes

* grpc-js timestamp conversion ([#755](#755)) ([9d24bd3](9d24bd3)), closes [#754](#754)
@stephenh
Copy link
Owner

stephenh commented Jan 7, 2023

🎉 This issue has been resolved in version 1.137.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants