Fix missing TypeScript return type for `serverStreaming` calls. #1167

lukasmoellerch · 2021-11-11T12:01:34Z

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.

In the generated typescript code this will ensure that the generic parameters will be infered from the values passed to the constructor

linux-foundation-easycla · 2021-11-11T12:01:38Z

The committers are authorized under a signed CLA.

✅ Lukas Möller (7ec3b9f)

sampajano

@lukasmoellerch Thanks so much for the fix! This is pretty smart :D Should be a great improvement! 😃

Just a few thoughts:

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?

These are the relevant lines if i read correctly:

grpc-web/javascript/net/grpc/web/generator/grpc_generator.cc

Lines 618 to 622 in 1efe741

    
           printer->Print(vars, "$js_method_name$(\n"); 
        
           printer->Indent(); 
        
           printer->Print(vars, 
        
                          "request: $input_type$,\n" 
        
                          "metadata?: grpcWeb.Metadata) {\n");

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

grpc-web/net/grpc/gateway/examples/echo/ts-example/client.ts

Lines 112 to 124 in 1efe741

    
           this.stream.on('data', (response: ServerStreamingEchoResponse) => { 
        
             EchoApp.addRightMessage(response.getMessage()); 
        
           }); 
        
           this.stream.on('status', (status: grpcWeb.Status) => { 
        
             if (status.metadata) { 
        
               console.log('Received metadata'); 
        
               console.log(status.metadata); 
        
             } 
        
           }); 
        
           this.stream.on('error', (err: grpcWeb.RpcError) => { 
        
             EchoApp.addRightMessage( 
        
                 'Error code: ' + err.code + ' "' + err.message + '"'); 
        
           });

FYI, you have to comment option 1 and uncomment option 2 to get Typescript input:

grpc-web/net/grpc/gateway/examples/echo/ts-example/client.ts

Lines 26 to 27 in 1efe741

    
           // Option 2: import_style=typescript 
        
           // import {EchoServiceClient} from './EchoServiceClientPb';

Thanks! :)

sampajano · 2021-11-13T03:04:04Z

packages/grpc-web/index.d.ts

+                requestSerializeFn: (request: REQ) => Uint8Array,
+                responseDeserializeFn: (bytes: Uint8Array) => RESP);


Could you make the return type to match the Closure type definition here? The fact it's currently using Uint8Arry is implementation detail and should probably be omitted.

grpc-web/javascript/net/grpc/web/methoddescriptor.js

Line 32 in 1efe741

* @param {function(REQUEST): ?} requestSerializeFn

I am not quite sure why it would make sense to have these typed as wither unknown or any - if we allow more values here consumers will be allowed to create MethodDescriptor instances which break internal invariants, e.g. I could create a MethodDescriptor with a serialize function which returns a number and typescript won't complain, but during runtime it will break. In my opinion MethodDescriptor is part of the public interface and thus the constructor should only allow valid values to be passed to the constructor. Nevertheless I can totally understand if you want it to return an interface / unknown instead, maybe it's just a matter of personal preference.

if we allow more values here consumers will be allowed to create MethodDescriptor instances which break internal invariants

I get your sentiment in general.. Although because MethodDescriptor code is generated rather than specified by callers, this probably doesn't make a practical difference..

Nevertheless I can totally understand if you want it to return an interface / unknown instead, maybe it's just a matter of personal preference.

The main reason i want to keep this in sync with the Closure type annotation is that internally (Google) we're referring to the Closure type as source of truth, and only on Github we're also adding the TS type info. And because the internal types are subject to change, I don't want us to commit to stronger types in TS than we do in JS. If that makes sense :)

Ok sure, thanks for the explanation. I reverted the types of requestSerializeFn and responseDeserializeFn to any.

Thanks a lot :)

sampajano · 2021-11-13T03:04:16Z

packages/grpc-web/index.d.ts

+    getRequestSerializeFn(): (request: REQ) => Uint8Array;
+    getResponseDeserializeFn(): (bytes: Uint8Array) => RESP;


Same as above :)

lukasmoellerch · 2021-11-13T11:35:12Z

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 · 2021-11-13T11:59:50Z

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.

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 :)

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

Fantastic! Thanks for verifying! :)

sampajano · 2021-11-19T01:12:13Z

packages/grpc-web/index.d.ts

+                requestSerializeFn: (request: REQ) => Uint8Array,
+                responseDeserializeFn: (bytes: Uint8Array) => RESP);


if we allow more values here consumers will be allowed to create MethodDescriptor instances which break internal invariants

I get your sentiment in general.. Although because MethodDescriptor code is generated rather than specified by callers, this probably doesn't make a practical difference..

Nevertheless I can totally understand if you want it to return an interface / unknown instead, maybe it's just a matter of personal preference.

The main reason i want to keep this in sync with the Closure type annotation is that internally (Google) we're referring to the Closure type as source of truth, and only on Github we're also adding the TS type info. And because the internal types are subject to change, I don't want us to commit to stronger types in TS than we do in JS. If that makes sense :)

- Adds attribute type declaratiosn to method descriptor - Adds return type declaration to server streaming call

…ellerch/grpc-web into ts-method-descriptor-constructor

lukasmoellerch · 2021-11-19T10:30:19Z

@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 · 2021-11-19T20:04:17Z

@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 :)

sampajano

@lukasmoellerch Thanks again for this great change! And your patience during code review! LGTM now! :)

lukasmoellerch · 2021-11-19T22:13:30Z

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 · 2021-11-19T23:06:13Z

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 · 2022-01-17T23:55:04Z

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

sampajano · 2022-01-21T18:48:35Z

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

sampajano · 2022-03-02T04:35:54Z

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

Add strongly types to MethodDescriptor constructor

7ec3b9f

In the generated typescript code this will ensure that the generic parameters will be infered from the values passed to the constructor

Replace method return types of MethodDescriptor

7b149dd

lukasmoellerch changed the title ~~Add strongly types to MethodDescriptor constructor~~ Add types to MethodDescriptor constructor Nov 11, 2021

sampajano requested review from sampajano and stanley-cheung November 13, 2021 00:33

stanley-cheung added the kokoro:run label Nov 13, 2021

grpc-kokoro removed the kokoro:run label Nov 13, 2021

sampajano reviewed Nov 13, 2021

View reviewed changes

sampajano reviewed Nov 19, 2021

View reviewed changes

Merge branch 'master' into ts-method-descriptor-constructor

5092589

sampajano added the kokoro:run label Nov 19, 2021

grpc-kokoro removed the kokoro:run label Nov 19, 2021

lukasmoellerch added 4 commits November 19, 2021 10:56

Generate type annotations in ts mode

177aebc

- Adds attribute type declaratiosn to method descriptor - Adds return type declaration to server streaming call

Revert serialize / deserialize fn ts type to any

c20a5f5

Merge branch 'ts-method-descriptor-constructor' of github.com:lukasmo…

1f49a43

…ellerch/grpc-web into ts-method-descriptor-constructor

add req / resp type, revert serialize / deserialize changes

f063db0

sampajano approved these changes Nov 19, 2021

View reviewed changes

sampajano added the kokoro:run label Nov 19, 2021

grpc-kokoro removed the kokoro:run label Nov 19, 2021

Revert method descriptor type annotation change

358f188

sampajano added the kokoro:run label Nov 19, 2021

grpc-kokoro removed the kokoro:run label Nov 19, 2021

sampajano added the release notes: yes label Nov 19, 2021

sampajano changed the title ~~Add types to MethodDescriptor constructor~~ Fix missing TypeScript return type for serverStreaming calls. Nov 19, 2021

sampajano merged commit 6b1d1e9 into grpc:master Nov 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing TypeScript return type for `serverStreaming` calls. #1167

Fix missing TypeScript return type for `serverStreaming` calls. #1167

lukasmoellerch commented Nov 11, 2021 •

edited

Loading

linux-foundation-easycla bot commented Nov 11, 2021 •

edited

Loading

sampajano left a comment •

edited

Loading

sampajano Nov 13, 2021

lukasmoellerch Nov 13, 2021

sampajano Nov 19, 2021 •

edited

Loading

lukasmoellerch Nov 19, 2021

sampajano Nov 19, 2021

sampajano Nov 13, 2021

lukasmoellerch commented Nov 13, 2021

lukasmoellerch commented Nov 13, 2021

sampajano left a comment

sampajano Nov 19, 2021 •

edited

Loading

lukasmoellerch commented Nov 19, 2021

sampajano commented Nov 19, 2021 •

edited

Loading

sampajano left a comment •

edited

Loading

lukasmoellerch commented Nov 19, 2021

sampajano commented Nov 19, 2021 •

edited

Loading

chrisdrobison commented Jan 17, 2022

sampajano commented Jan 21, 2022

sampajano commented Mar 2, 2022

	printer->Print(vars, "$js_method_name$(\n");
	printer->Indent();
	printer->Print(vars,
	"request: $input_type$,\n"
	"metadata?: grpcWeb.Metadata) {\n");

	this.stream.on('data', (response: ServerStreamingEchoResponse) => {
	EchoApp.addRightMessage(response.getMessage());
	});
	this.stream.on('status', (status: grpcWeb.Status) => {
	if (status.metadata) {
	console.log('Received metadata');
	console.log(status.metadata);
	}
	});
	this.stream.on('error', (err: grpcWeb.RpcError) => {
	EchoApp.addRightMessage(
	'Error code: ' + err.code + ' "' + err.message + '"');
	});

	// Option 2: import_style=typescript
	// import {EchoServiceClient} from './EchoServiceClientPb';

		requestSerializeFn: (request: REQ) => Uint8Array,
		responseDeserializeFn: (bytes: Uint8Array) => RESP);

		getRequestSerializeFn(): (request: REQ) => Uint8Array;
		getResponseDeserializeFn(): (bytes: Uint8Array) => RESP;

Fix missing TypeScript return type for serverStreaming calls. #1167

Fix missing TypeScript return type for serverStreaming calls. #1167

Conversation

lukasmoellerch commented Nov 11, 2021 • edited Loading

linux-foundation-easycla bot commented Nov 11, 2021 • edited Loading

sampajano left a comment • edited Loading

Choose a reason for hiding this comment

sampajano Nov 13, 2021

Choose a reason for hiding this comment

lukasmoellerch Nov 13, 2021

Choose a reason for hiding this comment

sampajano Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

lukasmoellerch Nov 19, 2021

Choose a reason for hiding this comment

sampajano Nov 19, 2021

Choose a reason for hiding this comment

sampajano Nov 13, 2021

Choose a reason for hiding this comment

lukasmoellerch commented Nov 13, 2021

lukasmoellerch commented Nov 13, 2021

sampajano left a comment

Choose a reason for hiding this comment

sampajano Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

lukasmoellerch commented Nov 19, 2021

sampajano commented Nov 19, 2021 • edited Loading

sampajano left a comment • edited Loading

Choose a reason for hiding this comment

lukasmoellerch commented Nov 19, 2021

sampajano commented Nov 19, 2021 • edited Loading

chrisdrobison commented Jan 17, 2022

sampajano commented Jan 21, 2022

sampajano commented Mar 2, 2022

Fix missing TypeScript return type for `serverStreaming` calls. #1167

Fix missing TypeScript return type for `serverStreaming` calls. #1167

lukasmoellerch commented Nov 11, 2021 •

edited

Loading

linux-foundation-easycla bot commented Nov 11, 2021 •

edited

Loading

sampajano left a comment •

edited

Loading

sampajano Nov 19, 2021 •

edited

Loading

sampajano Nov 19, 2021 •

edited

Loading

sampajano commented Nov 19, 2021 •

edited

Loading

sampajano left a comment •

edited

Loading

sampajano commented Nov 19, 2021 •

edited

Loading