grpc / grpc-web

gRPC for Web Clients
https://grpc.io
Apache License 2.0
8.63k stars 765 forks source link

Wrong return values for setters when generating .d.ts typings #917

Open p3337 opened 4 years ago

p3337 commented 4 years ago

Aloha!

When I generate .d.ts typings with any grpc-web version > 1.0.7 all setter-functions now return their class rather than void.

E.g. what I expect: setToken(value: Uint8Array | string): void;

What is generated: setToken(value: Uint8Array | string): SaltEntry

I guess this is because of a bug introduced with commit #747 (line 924, 927).

piercetrey-figure commented 4 years ago

I was literally just typing up a ticket for the same thing and this didn't come up in the search when I first looked, I'll just paste in here what I had in case it is helpful 😄

The typescript type definitions for a field setter seem to indicate a fluent interface, where the message object itself is returned (allowing for chaining), when in fact nothing is returned.

Here is an Example from a generated UUID type with a simple string value field (referencing the setValue method in particular):

export class UUID extends jspb.Message {
  getValue(): string;
  setValue(value: string): UUID;

  serializeBinary(): Uint8Array;
  toObject(includeInstance?: boolean): UUID.AsObject;
  static toObject(includeInstance: boolean, msg: UUID): UUID.AsObject;
  static serializeBinaryToWriter(message: UUID, writer: jspb.BinaryWriter): void;
  static deserializeBinary(bytes: Uint8Array): UUID;
  static deserializeBinaryFromReader(message: UUID, reader: jspb.BinaryReader): UUID;
}

I think that this definition would indicate that you could do something like the following:

const uuid: UUID = new UUID().setValue('uuid_string_here');

When in fact, the implementation of this method is as follows:

/** @param {string} value */
proto.UUID.prototype.setValue = function(value) {
  jspb.Message.setProto3StringField(this, 1, value);
};

Which has a void return type. In the example above, this would result in uuid being undefined.

Obviously, you can still do this in multiple lines as follows:

const uuid = new UUID();
uuid.setValue('uuid_string_here');

So this isn't really preventing anyone from doing anything probably, but it is a bit confusing if you are trusting autocomplete 😄 A fluent interface would be nice though, allow things to be a bit more streamlined.

I identified the same commit that @peeet referenced above, so I think he is correct. I'm new to grpc-web, so I don't know if I can be of any help or not, but figured I would document this here in case some improvement can be made.

jack-tutor commented 2 years ago

I also have this issue, but only when running with protoc on linux. protoc on mac generates javascript code that matches the typescript definitions (the fluent interface), but when generating protoc on linux, the javascript code does not match the typescript definition. This seems a very major bug to me.

cocreature commented 2 years ago

I ran into the same issue. In my case it was caused by an outdated protoc (not protoc-gen-grpc-web) version. I upgraded from protoc 3.8 to 3.19 and that generates a return and stuff works properly.