Support for grpc-node

[Multiply]

Would it be feasible to support service=grpc-node as well as service=grpc-web?

[Dig-Doug]

I think it would be doable, I'm happy to accept a PR to add support for it.

I think it would make sense to have different rules/macros, e.g. typescript_proto_library, typescript_grpc_web_library, grpc_node_library, etc. that generate the typescript protos vs the grpc services. This would be similar to how the c++ rules are structured. All of the rules could all use the same implementation under the hood with different input params.

[Multiply]

I got something working 10 minutes after posting my issue, although it required a smaller change in ts-protoc-gen as well.
It always generates the base d.ts, and optionally a single service.

I assume we want ts-protoc-gen to just generate the base d.ts+friends, and then optionally a use the other rules, to just generate the grpc-web / grpc-node services, right?

[Multiply]

Waiting for the outcome of improbable-eng/ts-protoc-gen#211 to start writing the implementation here. The proposal would also make the implementation here simpler, I reckon.

[Dig-Doug]

I assume we want ts-protoc-gen to just generate the base d.ts+friends, and then optionally a use the other rules, to just generate the grpc-web / grpc-node services, right?

Yes, something like:

# Generates *.d.ts for the protos
typescript_proto_library(
  name = "pizza_ts_proto",
  ...
)

# Generates grpc-web service code
typescript_grpc_web_library(
  name = "pizza_grpc_web_proto",
  ...
)

# Generates grpc-node service code
typescript_grpc_node_library(
  name = "pizza_grpc_node_proto",
  ...
)

Waiting for the outcome ...

Sounds good.

[Multiply]

@Dig-Doug Would it make more sense to add a generate attr that takes label_list with default of base?

It matches more the approach I ended up working on here: improbable-eng/ts-protoc-gen#212

Example:

typescript_proto_library(
    name = "example",
    srcs = glob(["**/*.proto"]),
    generate = [
        "base",
        "grpc-web",
    ],
)

The above would generate the same thing as typescript_proto_library does today.

[Dig-Doug]

One issue I can think of with that approach is users may not want to include all of the generated code in all of there builds. For example, I may have two apps A and B, one using grpc-node and one using gprc-web. If I depend on the single "example" rule, I get both sets of generated code. Having two different targets makes it very clear what is generated by each one.

Another thing is that specifying raw string input params is not ideal because it's error prone: there can be typos, etc. However, under the hood you could still have a single implementation with input params - just hide those details from the user using a macro. E.g.

# typescript_grpc_web_library.bzl
# Define a macro to hide the input params
def typescript_grpc_web_library(name):
  typescript_proto_library_impl(
    name = name,
    generate = "grpc-web",
  )

# user's BUILD.bazel
typescript_grpc_web_library(
  name = "pizza_grpc_web_proto",
  ...
)

Finally, the main java and c++ rules also separate the libraries, e.g. java_proto_library from java_grpc_library, so I think it would be good to follow the same pattern for consistency.

[Multiply]

I have something working locally now, that adds typescript_grpc_node_library and typescript_grpc_web_library

It is breaking backwards compatibility, as typescript_proto_library no longer generates the grpc-web part. Is that okay?

[Dig-Doug]

That's fine, it doesn't need to be backwards compatible - you'll just need to document how people should update example.

[kellycampbell]

@Multiply Is there a commit or PR for this which I could cherry-pick to try out?

[thesayyn]

@kellycampbell You can check this out https://github.com/thesayyn/protoc-gen-ts

[Multiply]

We ended up a different route dropping typescript entirely for now.
I pushed my work in progress to my fork, but it's nowhere near what we've discussed here.

If we revisit typescript down the road, I might be allocated more time to flesh the last details out.