Add bazel rule closure_grpc_web_library #219

Yannic · 2018-08-04T23:15:31Z

No description provided.

stanley-cheung · 2018-08-05T00:44:17Z

Wow, this is great! Thanks for the contribution! I will take a closer look shortly. Will let test run first.

stanley-cheung · 2018-08-05T00:46:44Z

bazel/closure_grpc_web_library.bzl

+      ctx.attr._grpc_web_clientreadablestream,
+      ctx.attr._grpc_web_error,
+      ctx.attr._grpc_web_grpcwebclientbase
+  ]


I wonder why only these 4 classes? Why not Status and StatusCode etc?

We only need to add the directly goog.required classes to deps, Status, StatusCode, etc are included as transitive deps by those 4 classes.
https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/grpc_generator.cc#L409
(Only mode GrpcWeb seems to be open source).

stanley-cheung · 2018-08-05T00:47:36Z

bazel/closure_grpc_web_library.bzl

+        ),
+        "_grpc_web_grpcwebclientbase": attr.label(
+            default = Label("//javascript/net/grpc/web:grpcwebclientbase"),
+        ),


Ditto, why just these 4 classes?

stanley-cheung · 2018-08-05T00:48:59Z

WORKSPACE

-    sha256 = "a80acb69c63d5f6437b099c111480a4493bad4592015af2127a2f49fb7512d8d",
-    strip_prefix = "rules_closure-0.7.0",
+    sha256 = "248a7a98eb3962d9f0013e543ea79c5063a83bac7af349ebf412d844e6ab3035",
+    strip_prefix = "rules_closure-53f2cab21fa6c608f32f114387d88ffd7868c5fc",


Why this particular commit? Something new available after 0.7.0?

This commit includes bazelbuild/rules_closure#278, which is required for this because closure_proto_aspect, which we need to generate the normal proto js files, is not exposed by rules_closure (Everything which starts with _ is private in bazel).

So we can only merge this PR after bazelbuild/rules_closure#278 got merged right?

Technically you can merge this, you'll just be dependent on a commit that's not in rules_closure's master branch (until it gets merged, that is).

It's exactly like @oferb said, there is no technical reason to block this until the other PR is merged.

Yannic · 2018-08-05T01:07:59Z

bazel/closure_grpc_web_library.bzl

+      ctx.attr._grpc_web_clientreadablestream,
+      ctx.attr._grpc_web_error,
+      ctx.attr._grpc_web_grpcwebclientbase
+  ]


We only need to add the directly goog.required classes to deps, Status, StatusCode, etc are included as transitive deps by those 4 classes.
https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/grpc_generator.cc#L409
(Only mode GrpcWeb seems to be open source).

Yannic · 2018-08-05T01:08:06Z

bazel/closure_grpc_web_library.bzl

+        ),
+        "_grpc_web_grpcwebclientbase": attr.label(
+            default = Label("//javascript/net/grpc/web:grpcwebclientbase"),
+        ),


Yannic · 2018-08-05T01:11:03Z

WORKSPACE

-    sha256 = "a80acb69c63d5f6437b099c111480a4493bad4592015af2127a2f49fb7512d8d",
-    strip_prefix = "rules_closure-0.7.0",
+    sha256 = "248a7a98eb3962d9f0013e543ea79c5063a83bac7af349ebf412d844e6ab3035",
+    strip_prefix = "rules_closure-53f2cab21fa6c608f32f114387d88ffd7868c5fc",


This commit includes bazelbuild/rules_closure#278, which is required for this because closure_proto_aspect, which we need to generate the normal proto js files, is not exposed by rules_closure (Everything which starts with _ is private in bazel).

Yannic · 2018-08-05T01:14:26Z

bazel/closure_grpc_web_library.bzl

+        ),
+        'import_style': attr.string(
+            default = 'closure',
+            values = ['closure', 'commonjs'],


I think I'd like to remove commonjs as a possible value for now. Most of rules_closure doesn't work with commonjs anyway.

Is it because the output of this rule is expected to mostly feed into other closure_js_library rule, which expects goog.require() style includes anyways?

The restriction to goog.require() is required because closure_proto_{aspect,library} (which generates the protobuf messages for us to avoid duplicate provide errors) does not support commonjs import style (yet). I will create a follow-up PR once this is fixed in rules_closure (Maybe we can create an issue for this and assign it to me).

stanley-cheung · 2018-08-05T02:21:38Z

I just want to confirm the goal of this:

The output of this closure_grpc_web_library rule should be able to feed into other rules, like a closure_js_library.
We should see the <proto>.grpc.js output in the bazel-bin directory somewhere?

stanley-cheung · 2018-08-05T02:39:55Z

scripts/kokoro.sh

@@ -25,6 +25,7 @@ docker build -t grpc-web:ubuntu_16_04 \
 docker-compose build

 bazel test javascript/net/grpc/web/...
+bazel test net/grpc/gateway/examples/...


Looks like this needs to be bazel build for now, instead of bazel test. Since there's no test target for now, the exit status of this command is 4, which invalidates the whole kokoro run.

Yannic · 2018-08-05T15:36:47Z

The output of this closure_grpc_web_library rule should be able to feed into other rules, like a closure_js_library.

Exactly. closure_grpc_web_library can be used, and is only really useful, as a dependency of closure_js_{*}.

We should see the .grpc.js output in the bazel-bin directory somewhere?

Well, the <proto>.grpc.js output may only be created if another target depends on it to reduce build time. This is similar to rules in rules_closure.

Yannic mentioned this pull request Aug 4, 2018

Export closure_proto_aspect and document closure_grpc_web_library bazelbuild/rules_closure#278

Merged

stanley-cheung added the kokoro:run label Aug 5, 2018

kokoro-team removed the kokoro:run label Aug 5, 2018

stanley-cheung reviewed Aug 5, 2018

View reviewed changes

Yannic commented Aug 5, 2018

View reviewed changes

Yannic force-pushed the rules_closure branch from 8b7cc6c to 37efbd8 Compare August 5, 2018 01:17

stanley-cheung added the kokoro:run label Aug 5, 2018

kokoro-team removed the kokoro:run label Aug 5, 2018

stanley-cheung reviewed Aug 5, 2018

View reviewed changes

Add bazel rule closure_grpc_web_library

76a4de8

Yannic force-pushed the rules_closure branch from 37efbd8 to 76a4de8 Compare August 5, 2018 15:13

stanley-cheung added the kokoro:run label Aug 5, 2018

kokoro-team removed the kokoro:run label Aug 5, 2018

stanley-cheung approved these changes Aug 6, 2018

View reviewed changes

stanley-cheung merged commit 9b7b2d5 into grpc:master Aug 6, 2018

Yannic deleted the rules_closure branch August 6, 2018 17:40

Yannic mentioned this pull request Aug 6, 2018

Use official release of rules_closure #223

Closed

Yannic mentioned this pull request Aug 22, 2018

Support CommonJS style imports for closure_grpc_web_library #260

Closed

stanley-cheung added the release notes: yes label May 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bazel rule closure_grpc_web_library #219

Add bazel rule closure_grpc_web_library #219

Yannic commented Aug 4, 2018

stanley-cheung commented Aug 5, 2018

stanley-cheung Aug 5, 2018

Yannic Aug 5, 2018

stanley-cheung Aug 5, 2018

Yannic Aug 5, 2018

stanley-cheung Aug 5, 2018

Yannic Aug 5, 2018

stanley-cheung Aug 5, 2018

oferb Aug 5, 2018

Yannic Aug 5, 2018

Yannic Aug 5, 2018

Yannic Aug 5, 2018

Yannic Aug 5, 2018

Yannic Aug 5, 2018

stanley-cheung Aug 5, 2018

Yannic Aug 5, 2018

stanley-cheung commented Aug 5, 2018 •

edited

Loading

stanley-cheung Aug 5, 2018

Yannic Aug 5, 2018

Yannic commented Aug 5, 2018

Add bazel rule closure_grpc_web_library #219

Add bazel rule closure_grpc_web_library #219

Conversation

Yannic commented Aug 4, 2018

stanley-cheung commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanley-cheung commented Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic commented Aug 5, 2018

stanley-cheung commented Aug 5, 2018 •

edited

Loading