fix: grpc-js example

[reggiemcdonald]

Which problem is this PR solving?

• Fixes grpc-js example has missing dependency #1357
• The current example implementation uses pb files that were compiled for grpc and thus incompatible with grpc-js. This PR will resolve these incompatibilities.

Short description of the changes

• Recompiled the proto file with grpc-tools. Bellow was the command used:

# Assuming that `helloworld.proto` is at the root of the example
npm install
npx grpc_tools_node_protoc \
  --js_out=import_style=commonjs,binary:./ \
  --grpc_out=grpc_js:./ \
  -I=./ helloworld.proto

grpc_tools_node_protoc provides the protoc interface, but the output is compatible with grpc-js

[reggiemcdonald]

/cc @nlehrer @danherr

[codecov]

Codecov Report

Merging #1362 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1362   +/-   ##
=======================================
  Coverage   93.18%   93.18%           
=======================================
  Files         149      149           
  Lines        4228     4228           
  Branches      869      869           
=======================================
  Hits         3940     3940           
  Misses        288      288           

[dyladan]

Suggested change

	throw new Error('Expected argument of type helloworld.HelloReply');
	throw new Error('Expected argument of type helloworld_pb.HelloReply');

[dyladan]

Why make this a var?

[dyladan]

All of the changes to this file appear to be unrelated to the purpose of the PR?

[reggiemcdonald]

This is the recompiled output from protoc. Would you prefer me to manually edit the file to make it work? There would be a much smaller diff if I make the edits manually.

[dyladan]

I didn't realize this was generated

[reggiemcdonald]

Yea both the grpc and grpc-js repos use generated code from protoc, using a proto file from a grpc example on github.com/grpc/grpc

[dyladan]

Many places where const rewritten to var. Any reason for this?

[reggiemcdonald]

I used the commonjs flag to protoc. Maybe theres a different flag to get let and const

[dyladan]

I'm not overly familiar with protoc so I'm not sure.

[obecny]

would it be possible to use @grpc/proto-loader instead and then remove the generated files completely ?

[reggiemcdonald]

would it be possible to use @grpc/proto-loader instead and then remove the generated files completely ?

The proto file used in this example is from github.com/grpc/grpc. So we would need to commit that file to the repository for the proto-loader option to work.

To keep the diff down, I could edit the existing file manually - you can see what that would look like here. It works, and it's much smaller. It just isn't what the protobuf compiler would output.

[obecny]

would it be possible to use @grpc/proto-loader instead and then remove the generated files completely ?

The proto file used in this example is from github.com/grpc/grpc. So we would need to commit that file to the repository for the proto-loader option to work.

To keep the diff down, I could edit the existing file manually - you can see what that would look like here. It works, and it's much smaller. It just isn't what the protobuf compiler would output.

I have been in the same place recently when I was adding support for proto for collector exporter. Started with generated files, but it turned out I can simply load the proto files and then use the package without the need to generate the files. Much easier to maintain, less errors and code.

[dyladan]

would it be possible to use @grpc/proto-loader instead and then remove the generated files completely ?

The proto file used in this example is from github.com/grpc/grpc. So we would need to commit that file to the repository for the proto-loader option to work.

To keep the diff down, I could edit the existing file manually - you can see what that would look like here. It works, and it's much smaller. It just isn't what the protobuf compiler would output.

I don't think it's an issue to check in the proto file to this example.

The manually edited version also looks ok to me, so for me either is fine.

[markwolff]

+1 to switching these examples over to protoloading the sample .proto files instead of these codegenerated js files. I ported the plugin tests use .proto + @grpc/proto-loader, so something similar could be done with these samples.

The only reason the old grpc examples had the codegen example at all is because that grpc module supported several different ways of setting up a client. For grpc-js, all are deprecated except for .proto + @grpc/proto-loader.

[reggiemcdonald]

Why don't I use the manually edited version for this issue to get the example working, and then open a new issue to move both the grpc and grpc-js examples over to protoloader - which I would be happy to work on 🙂

[reggiemcdonald]

closing in favour of #1364