Fix missing TypeScript return type for `serverStreaming` calls.

[lukasmoellerch]

Should fix #950

In the generated typescript code this will ensure that the generic parameters will be inferred from the values passed to the constructor. This should only make a difference when the import_style setting is set to typescript. Afterwards in the following code snippet the the response parameter will correctly infer to ServerStreamingEchoResponse, whereas previously it would be typed as unknown.

import * as grpcWeb from 'grpc-web';

import {EchoRequest, EchoResponse,
        ServerStreamingEchoRequest,
        ServerStreamingEchoResponse} from './generated/echo_pb';
import {EchoServiceClient} from './generated/EchoServiceClientPb';

const echoService = new EchoServiceClient('http://localhost:8080', null, null);

let call2 =
  echoService.serverStreamingEcho(new ServerStreamingEchoRequest(), {});

call2
  .on('data', (response) => {
  })
  .on('error', (error: grpcWeb.RpcError) => {
  });

I am not entirely sure where tests for the typescript mode are located, I was only able to finde the tsc_tests, but they seem to be using the dts generator.

[linux-foundation-easycla[bot]]

The committers are authorized under a signed CLA.

• :white_check_mark: Lukas Möller (7ec3b9f7b369a608ad17666ccc9a13eb8d355b60)

[lukasmoellerch]

Thanks for the quick response! Regarding your first point:

I think part of the problem is that the generated client stub (EchoServiceClientPb.ts in your example) doesn't specify return type. I wonder if that can be fixed as well?

We could add a return type declaration here, but the inferred return type is already correct I think: Because serverStreaming returns a ClientReadableStream<RESP> as per its declaration in index.d.ts the inferred return type will also be ClientReadableStream with the generic parameter being inferred from the method descriptor. We could still add a direct annotation there, but I don't think that it will change the type-checking in any way.

I wonder if you could just remove type specifications in our TS example to help verify that type reference indeed works?

Yes, I think we should be able to remove those. I'll quickly try that, thanks for the hint.

[lukasmoellerch]

Yes, I think we should be able to remove those. I'll quickly try that, thanks for the hint.

Ok, I did some quick testing: After convincing typescript to use the new type declarations with the following tsconfig file:

{
  "compilerOptions": {
    "target": "es6",
    "module": "commonjs",
    "strict": true,
    "allowJs": true,
    "outDir": "./dist",
    "types": ["node"],
    "baseUrl": "./",
    "paths": {
      "grpc-web": ["../../../../../../packages/grpc-web/"]
    }
  },

  "include": [
    "client.ts",
    "echo_pb.js",
    "echo_pb.d.ts",
    "echo_grpc_web_pb.js",
    "echo_grpc_web_pb.d.ts",
    "EchoServiceClientPb.ts"
  ],
  "exclude": ["node_modules"]
}

I indeed get the correct parameter type inferred and the parameter type inferred correctly:

and the serverStreamingEcho method has the correct type as well:

EchoServiceClient.serverStreamingEcho(request: ServerStreamingEchoRequest, metadata?: grpcWeb.Metadata | undefined): grpcWeb.ClientReadableStream<ServerStreamingEchoResponse>

whereas previously I got the following error:

Type 'ClientReadableStream<unknown>' is not assignable to type 'ClientReadableStream<ServerStreamingEchoResponse>'.
  Type 'unknown' is not assignable to type 'ServerStreamingEchoResponse'

in the line where this.stream is assigned.

[lukasmoellerch]

@lukasmoellerch Thanks so much for the change again! Mostly LGTM after resolving the open comments :)

We could add a return type declaration here, but the inferred return type is already correct I think: Because serverStreaming returns a ClientReadableStream<RESP> as per its declaration in index.d.ts the inferred return type will also be ClientReadableStream with the generic parameter being inferred from the method descriptor. We could still add a direct annotation there, but I don't think that it will change the type-checking in any way.

Yup agreed that inference is already working after your change.. I just thought also adding it to the generated TS file will add even a stronger guarantee of type info (e.g. inference might be accidentally broken).

But I'm fine if we we leave this as is and maybe do a follow-up change later :)

Sure, I modified the code generator, it should now generate the type annotations for server streaming methods and also for the generic params of the method descriptors. I hope the latter change is okay.

[sampajano]

@lukasmoellerch Thanks so much for the change again! Mostly LGTM after resolving the open comments :)

We could add a return type declaration here, but the inferred return type is already correct I think: Because serverStreaming returns a ClientReadableStream<RESP> as per its declaration in index.d.ts the inferred return type will also be ClientReadableStream with the generic parameter being inferred from the method descriptor. We could still add a direct annotation there, but I don't think that it will change the type-checking in any way.

Yup agreed that inference is already working after your change.. I just thought also adding it to the generated TS file will add even a stronger guarantee of type info (e.g. inference might be accidentally broken). But I'm fine if we we leave this as is and maybe do a follow-up change later :)

Sure, I modified the code generator, it should now generate the type annotations for server streaming methods and also for the generic params of the method descriptors. I hope the latter change is okay.

Thanks a lot for the change! My personal preference is to omit the method descriptor types just because 1) users should rarely need to reference them directly, 2) this is a case where type inference becomes very clear after your change (whereas for the streaming endpoint it's more roundabout) :)

But since this is generated code so a little extra verbosity maybe doesn't hurt.. I'll leave it to you :)

[lukasmoellerch]

But since this is generated code so a little extra verbosity maybe doesn't hurt.. I'll leave it to you :)

On second thought I have to agree: I think keeping the generated code clean is probably preferable here. I reverted that change.

[sampajano]

On second thought I have to agree: I think keeping the generated code clean is probably preferable here. I reverted that change.

Oh cool sgtm! Thanks again for your contribution and patience through the reviews! :)

I'll cut a 1.3.1 release soon. I'm excited for this improvement! :D

(btw i've modified the name of the PR to better reflect the actual function of it :))

[chrisdrobison]

@sampajano Do you plan you cut a new release so this can be pushed out?

[sampajano]

Yes! I'm sorry about the delay! I'll aim to do this in the next week! 😃

[sampajano]

@chrisdrobison This should now be in release 1.3.1. thanks :)