Generate service files regardless of whether there are service definitions in the protos

[lx223]

This PR is to always generate both js and d.ts service files so that Bazel can rely on it always being present when service=true is used.

The detail changes are:

• Remove the if that changes if there are service definitions in the protos
• Regenerate the examples with the new code
• Update README to indicate that

Note: Some redundant code will still be included in those two files even if there are no service definitions. I didn't remove them as they are no used or dependent on so they shouldn't affect the final binary size anyway.

[easyCZ]

Can we not instead detect if the proto contains a service definition? This removes the almost empty files.

[jonnyreeves]

@lx223 Thanks for this pull, could you explain the motivation in a little more detail? Does bazel expect a 1-1 proto -> service definition, or is it something more nuanced (ie: the presence of a service block as hinted by @easyCZ)

Thanks.

[lx223]

Whether or not there is a service definition in a proto is easily visible to protoc and its plugins (if we are not considering grep-ing the content of the proto directly) but not so much to external binaries. And there is no easy way to ask protoc if it is going to generate service files.

And Bazel is very strict about what output files to expect. If the plugin writes service files only if there are service definitions in protos (which is usually what we should do), it would be very problematic for Bazel rules to deal with.

In short, a 1-n proto file -> service definition files (n doesn't have to be 1 but it should be a fixed number) is much easier for Bazel rules to deal with.

[jonnyreeves]

Thanks for clarifying @lx223. Please could you confirm that the go-grpc plugin also generates service stubs even if there are no services defined in the proto, or have they found a workaround?

[jonnyreeves]

This comment feels redundant for most use cases. I would prefer a unit test, with a descriptive name, which checks the required files are generated

[lx223]

Would be happy to add a unit test.

Maybe the last bit of this comment is not very fitting. But, I feel it would be good still to have this:

**Note** Both `js` and `d.ts` service files will be generated regardless of whether there are service definitions in the proto files.

What do you think?

[jonnyreeves]

Sgtm

[lx223]

Go is slightly different in the sense that both the proto and grpc generated code are put into one file. But, the 1 to n relationship is still respected in the sense that even if one puts nothing in a proto, it would still generate a .go file. And it is also the same case with GRPC gateway code-gen.

[jonnyreeves]

Thanks for confirming. I'm happy for this to land but would prefer an integration test over a comment in the readme

[easyCZ]

I really don't want service declarations which are empty to exist. As a user, I would consider this a bug or would be wondering if I'm doing something wrong when generating the protos.

In practice, nothing is stopping us from generating the service in the same file.

@lx223 Do you have a POC of the bazel plugin?

I believe it would be possible to have an optional argument on the bazel rule specifying if a service file should be generated. This would require a proto_lib declaration for each file but we could write a rule which would expand to these.

Also, the GenerationRequest proto already specifies if there will be a service generated, this is the same format the proto_lib rule operates on.

[easyCZ]

If we do choose to go with the empty service files then they probably should not even contain the current output, ie

var othercom_external_enum_pb = require("../othercom/external_enum_pb");
var grpc = require("grpc-web-client").grpc;

[lx223]

As I mentioned in the description of the PR, I left them in since they won't be used or affect the final JS app anyway. But, I am happy to remove them.

Could you please link proto_lib as I am not entirely sure what you are referring to?

I have written some POC Bazel rules which are working with the requested change in this PR if that's what you meant by Bazel plugin?

Yes, generating into the same file would work for the d.ts file. But the js file still won't work.

It is possible to specify parameters for Bazel rules. But, the parameters would more likely to be whether to generate service files for *.proto rather than whether to generate service files for a.proto but not for b.proto since whether a proto file would produce service files is opaque to Bazel rules.

I totally agree that having empty files is a little surprising usually. However, it is pretty common with code-gen and Bazel. There are many examples which do this, such as go-grpc-gateway and go-proto-validator.

[easyCZ]

proto_library

Just glancing over the existing ts-proto-library it appears they are generating multiple outputs for the same input.

[lx223]

Not sure if I am understanding you correct, do correct me if I am wrong. proto_library generates a descriptor set with a given list of protos. It is not necessary to generate one for each proto file. And we don't really need the descriptor set either way IMO.

Yep, it is definitely possible to generate multiple outputs for the same inputs as long as the rule knows what output files to expect, i.e. when we set service=true, we'd expect service files for all protos so that the rule can compile a list of output files for Bazel to count on being present. If there are sometimes service files and sometimes not for reasons external to the Bazel rule, it becomes problematic.

[jonnyreeves]

How about we add a flag? allow_empty

[lx223]

@jonnyreeves Have added the integration test. PTAL 😄

I wouldn't oppose it but I don't think it is necessary. These are generated code. As a user, I wouldn't really care what files are there as long as I can import things correctly. So overall, I don't think it is something worth over-engineering.

[easyCZ]

The way I'm reading it, the proto-library gives you a FileDescriptorSet proto which is the descriptor that ts-protoc-gen requires to compile the protos. Hence my reasoning was to use it as it is

1. compatible with the way you declare proto libraries
2. Already in a format that ts-protoc-gen can consume

[coltonmorris]

I worked around this in the current pull request by having the rule define the services that will be generated by its proto files:

typescript_proto_library(
  name = "generate",
  proto = ":proto",
  services = [
    "orphan",
  ],
)

[lx223]

@easyCZ Not entirely sure what you mean. The descriptor set is consumable by the protoc plugin. How is it related to whether Bazel knows if there are going to be service files?

@coltonmorris Thanks for the suggestion. I can see how it would work with a small number of protos. It might struggle a bit with a large number of protos and globs though.

[easyCZ]

The above was more concerned with why we need proto_library and how it should interact with protoc and ts-protoc-gen. Ie, since proto_library is the canonical way to declare proto dependencies, we should make sure any rule we write makes use of it (concern for a different PR, however).

I had a look and I can't find a quick solution for this. Let's go with this as is and I'm gonna investigate further if there's anything we could do. Thanks for taking the time to do this!

Sorry for holding up the PR, just trying to get a better sense of how it will fit in the larger ecosystem

[jonnyreeves]

Merging as is. Can't say I'm overjoyed about generating empty files but I also don't see any real world implications either.