Support CommonJS style imports for closure_grpc_web_library

[vmax]

It's similar to how closure_js_proto_library supports different import styles

cc @oferb

[Yannic]

Unfortunately, this won't work, and I think it will be confusing if we have this option.
closure_grpc_web_library is based on closure_proto_library, which does not support commonjs yet. #219 (comment)

[oferb]

In what way won't this work? It does generate commonjs imports.
Perhaps alternatively, we could create a commonjs_grpc_web_library?

[Yannic]

Yes, it does generate commons imports in the .grpc.js files, but this files will depend on proto files which do always use goog.{provide,require} https://github.com/bazelbuild/rules_closure/blob/master/closure/protobuf/closure_proto_library.bzl#L56

Adding a flag to closure_proto_library (not the old closure_js_proto_library which does not integrate well into Bazels protobuf ecosystem and has different problems) is tricky because everything explodes if someone mixes commonjs and goog.require style code. Adding a flag for commonjs to closure_proto_library wasn't a high priority because, afaik, it is not widely used by users of rules_closure.

commonjs_grpc_web_library is an alternative, but you will have to create commonjs_proto_library too.

[stanley-cheung]

@Yannic So I patched in this PR. And then I modified this BUILD rule: https://github.com/grpc/grpc-web/blob/master/net/grpc/gateway/examples/echo/BUILD.bazel#L10

to become:

closure_grpc_web_library(                                                                                     
    name = "echo_commonjs",                                                                                   
    import_style = "commonjs",                                                                                
    deps = [                                                                                                  
        ":echo_proto",                                                                                        
    ],                                                                                                        
)   

It builds properly with no error. It outputs a bazel-bin/net/grpc/gateway/examples/echo/echo.grpc.js file. The content is exactly like I'd expect if you run the protoc-gen-grpc-web protoc plugin directly with import_style=commonjs.

Yes the resulting file has this line

require('./echo_pb.js');   

But that's up to the user of this BUILD rule on how to integrate right? They may have another rule that outputs echo_pb.js to that same directory (by running a vanilla protoc --js_out=import_style=commonjs:. perhaps).

@vmax @oferb Can you confirm that's all you want?

[Yannic]

(Sorry, longish answer incoming)
I think there is some confusion about how closure_grpc_web_library works.

Yeah, the echo.grpc.js is generated just fine. However, it is not up to the user to provide the dependencies, they are generated by an aspect and currently always import_style=closure. While it would be trivial to generate these protobuf definitions with import_style=commonjs, there is a problematic edge case:

Assume we have the following build graph (yellow targets are generated by aspects):

In this case, it would not matter what import_style we use.
However, if we allow users to define the import_style, we may end up with the following build graph:

In this case, we have a_closure_proto (and the grpc/proto runtime, which we will ignore for brevity) two times in the build graph: One time with import_style=closure and one time with import_style=commonjs.
Now, the target some_library can (transitively) depend on a_closure_proto and a_closure_proto (commonjs), which is obviously problematic because we have two different versions of the same a_proto file in the same build graph. (Yes, it is possible to mix closure style code with commonjs style code). Since there is no infrastructure to catch this error at build time, and it will result in confusing and probably hard-to-debug errors at runtime, it was decided to disallow import_style=commonjs for now and decide what to do with when, or if, someone has a concrete use-case, which hasn't happened until now.

I hope this explains why it's not as easy to support commonjs as this PR suggests.

Regarding the closure_js_proto_library mentioned by @vmax: I read the code of this rule again, and I have serious doubts that this ever actually worked with import_style=commonjs since we always inject the runtime for import_style=closure https://github.com/bazelbuild/rules_closure/blob/master/closure/protobuf/closure_js_proto_library.bzl#L71.