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 typescript syntax generated for google.protobuf.Struct RPC replies in v1.88.0 #403

Closed
mkmarek opened this issue Nov 22, 2021 · 4 comments · Fixed by #407
Closed
Labels

Comments

@mkmarek
Copy link

mkmarek commented Nov 22, 2021

Hi, there seems to be a regression in v1.88.0 when it comes to decoding response data that are google.protobuf.Struct.

I made a small repository to replicate the issue: https://github.com/mkmarek/ts-proto-bug

For a service like this:

service Service {
    rpc Bar (BarRequest) returns (google.protobuf.Struct);
}

there is this kind of typescript code generated:

Bar(
      request: BarRequest
    ): Promise<{[key: string]: any} | undefined> {
      const data = BarRequest.encode(request).finish();
      const promise = this.rpc.request(
        
        "pkg.Service",
        "Bar",
        data
      );
      return promise.then(data => {[key: string]: any} | undefined.decode(new Reader(data)));
    }

the return promise.then(data => {[key: string]: any} | undefined.decode(new Reader(data))); has invalid syntax.

Versions used:

libprotoc 3.0.0
ts-proto v1.88.0

@stephenh
Copy link
Owner

Ah hrm, @boukeversteegh given this is related to google.protobuf.Struct, wdyt of ^? I can take a look later as well... Thanks for the report @mkmarek !

@boukeversteegh
Copy link
Collaborator

boukeversteegh commented Nov 22, 2021

Oh yes, I had similar issues in other places. The call-site where the TypeName is resolved assumes that it will always get back a ClassName for 'messages' (not primitives). However, since now we have several message types that don't resolve to class-names by default, you get these breaks.

The fix is easy though, the call to basicTypeName should specify {keepValueType: true}.

I will look tomorrow or wednesday if no-one else would. We probably need a test for this too.

@boukeversteegh
Copy link
Collaborator

couldn't help to have a quick look.

the problem is here I think, where responseType() should preserve the original value type, and not convert it to typescript.

normally, grpc services cannot return primitives (i think?), so the assumption makes sense.

const outputType = responseType(ctx, methodDesc);
const params = [...(options.context ? [code`ctx: Context`] : []), code`request: ${inputType}`];
const maybeCtx = options.context ? 'ctx,' : '';
let encode = code`${rawInputType}.encode(request).finish()`;
let decode = code`data => ${outputType}.decode(new ${Reader}(data))`;

@stephenh
Copy link
Owner

🎉 This issue has been resolved in version 1.90.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.

3 participants