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

Support Values as service return values #947

Closed
moufmouf opened this issue Oct 10, 2023 · 2 comments · Fixed by #951
Closed

Support Values as service return values #947

moufmouf opened this issue Oct 10, 2023 · 2 comments · Fixed by #951

Comments

@moufmouf
Copy link
Contributor

First of all, thanks a ton for ts-proto. This lib really is a game changer ❤️

I believe I found a bug in the way ts-proto handles "Value" object in services.

Here is a sample service:

service RoomApi {
    rpc readVariable(VariableRequest) returns (google.protobuf.Value);
}

See how readVariable returns directly a google.protobuf.Value ?

When I look at the code generated for the service, I have this:

export const RoomApiService = {
  readVariable: {
    path: "/roomApi.RoomApi/readVariable",
    requestStream: false,
    responseStream: false,
    requestSerialize: (value: VariableRequest) => Buffer.from(VariableRequest.encode(value).finish()),
    requestDeserialize: (value: Buffer) => VariableRequest.decode(value),
    responseSerialize: (value: any | undefined) => Buffer.from(Value.encode(value).finish()),
    responseDeserialize: (value: Buffer) => Value.decode(value),
  },
}

The responseSerialize is typed to "any", but Value.encode expects the value to be a "Value" object.
Same thing, responseDeserialize will return a "Value" object, but the end user probably expects a plain object instead.

As a result, when reading the result of a call to readVariable, the end user needs to manually call Value.unwrap.

The code looks like this:

const value = Value.unwrap(await client.readVariable({ /*...*/ });

Same thing server-side where we need to manually call Value.wrap.

I believe the correct implementation should generate something like:

    responseSerialize: (value: any | undefined) => Buffer.from(Value.encode(Value.wrap(value)).finish()),
    responseDeserialize: (value: Buffer) => Value.unwrap(Value.decode(value)),

What do you think?

Also, the same issue happens with streamed return values. For instance:

service RoomApi {
    rpc listenVariable(VariableRequest) returns (stream google.protobuf.Value);
}

But, the issue does not happen if the Value is part of a message:

message DispatchGlobalEventRequest {
  string name = 1;
  google.protobuf.Value value = 2; // This is correctly serialized/unserialized to "any"
}

Bonus: could it be possible have an option to cast protobuf Value to Typescript unknown (instead of any) for type safety?

@stephenh
Copy link
Owner

Hi @moufmouf ; yeah, I am not entirely surprised this is an issue.

As you noticed, we have code inside of the main "generate encode/decode" methods to handle value types:

https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L1024

Doesn't seem to be using it:

https://github.com/stephenh/ts-proto/blob/main/src/generate-grpc-js.ts#L91

https://github.com/stephenh/ts-proto/blob/main/src/encode.ts#L7

If you'd like to submit a PR that moves/copy/pastes the Value-related code into the encode/decode handling, that'd be great! Thanks!

@stephenh stephenh changed the title Issue while serializing a Value object Support Values as service return values Oct 13, 2023
@moufmouf
Copy link
Contributor Author

Hey @stephenh ,

Thanks a lot for this quick response. Thanks to your help, it was easy enough to fix the issue.

PR is here : #951

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