RFC: The future of this package

[jonnyreeves]

Overview

This package (ts-protoc-gen) has always conflated two distinct use-cases:

1. Generate TypeScript definitions (.d.ts) to accompany the JavaScript generated by protoc
2. Generate the JavaScript and TypeScript definitions for use with improbable-eng/grpc-web

In the light of recent forks, such as adding support for flowtype, I feel it may be wise to consider splitting these two use-cases out into two distinct protoc plugins.

1. protoc-gen-tsd (usage: --tsd_out=$OUT)
2. protoc-gen-improbable-grpc-web (usage: --improbable-grpc-web_out=dts:$OUT)

This renaming would also help create a distinction between the official grpc-web offering from google, and the improbable-eng/grpc-web offering from Improbable.

Migration Path

1. This repo (and associated npm package) could potentially continue to exist and wrap protoc-gen-tsd and protoc-gen-improbable-grpc-web to avoid breaking consumer workflows until such a point that they can be deprecated and migrated.
2. The code would need to be split; I can see two possible approaches:
   a.) Create 2 or more repositories under improbable-eng for each protoc plugin (and possibly one for shared code)
   b.) Create a single mono-repository using a tool like lerna to manage the node modules.

Misc

It may make more sense to join efforts with agreatfool/grpc_tools_node_protoc_ts (cc @agreatfool) who has built out their own protoc compiler for generating TypeScript definitions. Although a full audit of feature-sets would need to follow.

cc @easyCZ @MarcusLongmuir @johanbrandhorst @chrisgervang

[johanbrandhorst]

Great initiative, agreed that it will be a good thing for users to have more clarity on these two use cases.

[jonnyreeves]

Calling @Dig-Doug, @coltonmorris as our resident bazel experts; please could you help me understand the implications of splitting ts-protoc-gen into 2 distinct tools (see overview above) with regards to the bazel build rules in this repo? Your input on how to manage such a migration would be appreciated.

[jonnyreeves]

I've created the above issue in the grpc/grpc-web project to see if we can avoid creating the protoc-gen-tsd plugin mentioned in this issues overview.

[Dig-Doug]

There are two main parts to the current bazel setup:

1. Rules needed to build the protoc plugins under bazel, e.g. this target

2. Rules for generating typescript definitions and services stubs, mostly in this file

# 1

No matter how you split up your protoc plugins, the new repo structure will need to have the bazel setup (replacing # 1) to run them in # 2. Looking at your suggestions, I don't see any issues with either a) or b): bazel can pull in tools from multiple repos or reference mutliple tools from the same repo.

With respect to migrating, this code is all internal. Clients shouldn't be referencing these targets directly.

# 2

Based on the new structure, the code for # 2 will need to be updated to point to the new tools (e.g. here). This code can live in a different repo¹ than the plugins or in the same repo. Note that the rules need to reference all of the plugins, so make sure the rules are in a place that doesn't cause circular dependencies.

Migrating: This code is what clients interact with. While the API of the rules won't be changing, the location will. For clients, this means changing the WORKSPACE file:

http_archive(
    # NOTE: The name should be changed to match new location
    name = "<new-name>",
    urls = ["https://github.com/<new-location>"],
)

and load calls in BUILD files:

# NOTE: Since the http_archive's name has changed, load calls need to be updated:
load("@<new-name>//:defs.bzl", "typescript_proto_library")

Once you have decided on the location of the new rules, you can also add a warning to the existing implementation that they're deprecated. The warning will get printed during the build. However, clients will only see this warning if they've updated the rules to a later version.

¹ I noticed the other day that the rules_proto library is referencing the ts-protoc-gen code here -- though I don't think it currently works. rules_protobuf and its successor, rules_proto, is a popular library for using protos/grpc with bazel. If you'd like to split out the bazel rules in addition to the protoc plugins, you should consider moving them there.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Dig-Doug]

Update on this: Recently there have been some new features added to rules_nodejs that could eliminate the need to have any bazel setup in this repo.

I took a look at the issues today and noticed that there are a bunch related to the bazel rules, not the protoc plugin. With the current setup it's a bit difficult for me to keep an eye on open issues, PRs, etc. @jonnyreeves - I was wondering what you think of me moving the bazel rules to a separate repository?

[jonnyreeves]

I would very much be in favour of this 👍

…

On Sun, 20 Oct 2019, 16:43 Dig-Doug, ***@***.***> wrote: Update on this: Recently there have been some new features added to rules_nodejs <https://github.com/bazelbuild/rules_nodejs/releases> that could eliminate the need to have any bazel setup in this repo. I took a look at the issues today and noticed that there are a bunch related to the bazel rules, not the protoc plugin. With the current setup it's a bit difficult for me to keep an eye on open issues, PRs, etc. @jonnyreeves <https://github.com/jonnyreeves> - I was wondering what you think of me moving the bazel rules to a separate repository? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#145?email_source=notifications&email_token=AABQ36JLEVVSXC6PT6DUT5TQPR4C7A5CNFSM4GKJ4V6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBYM67A#issuecomment-544264060>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABQ36JGRDNMUP7Z4ZO45XTQPR4C7ANCNFSM4GKJ4V6A> .

[Dig-Doug]

Great, I have created rules_typescript_proto and submitted a PR to clean up the code here.

[Raiondesu]

Is this issue still relevant?