Add ts_proto_library rule. #193

alexeagle · 2018-04-27T20:48:30Z

Add example that uses it under a karma test.

Fixes #61

/cc @kellycampbell

gregmagolan · 2018-04-27T20:53:09Z

Fix windows CI?

gregmagolan

LGTM

alexeagle · 2018-04-30T22:27:07Z

/cc @cgrushko

Add example that uses it under a karma test. Fixes bazelbuild#61

mrmeku

Looks great! I'm excited to see how you'll remove the need for the rollup.config

mrmeku · 2018-04-30T23:08:44Z

examples/protocol_buffers/car.proto

+message Car {
+  string make = 1;
+  string model = 2;
+  int32 year = 3;


You might consider exercising your test with a message containing an int64. pbjs has a couple of modes for handing int64 in JS and one of the requires that the long node_module be used. We should test that we integrated that scenario correctly

thanks, good point

mrmeku · 2018-04-30T23:09:39Z

examples/tsconfig.json

@@ -1,7 +1,8 @@
 {
  "compilerOptions": {
    "strict": true,
-    "lib": ["es2015.promise", "dom", "es5"]
+    "lib": ["es2015.promise", "dom", "es5"],
+    "rootDirs": [".", "../bazel-bin/examples"]


Is this for editor support of generated files?

Yes, exactly, I'll add a comment

cgrushko · 2018-04-30T23:25:45Z

cc @iirina

alexeagle · 2018-05-01T03:40:06Z

/cc @pherl - anyone from your team care to be in the loop for Bazel / TypeScript proto usage?

gregmagolan · 2018-05-01T17:38:03Z

internal/devserver/ts_devserver.bzl

@@ -106,6 +116,7 @@ ts_devserver = rule(
        "serving_path": attr.string(),
        "data": attr.label_list(allow_files = True, cfg = "data"),
        "static_files": attr.label_list(allow_files = True),
+        "bootstrap": attr.label_list(allow_files = [".js"]),
        # User scripts for the devserver to concat before the source files
        "scripts": attr.label_list(allow_files = True),


Suppose it is correct to limit scripts to [".js"] as well

…efore requirejs

jbedard · 2018-05-03T20:40:52Z

internal/protobufjs/ts_proto_library.bzl

+      type_blacklisted_declarations = [],
+      es5_sources = depset([js_es5]),
+      es6_sources = depset([js_es6]),
+      transitive_es5_sources = depset(),


I tried this PR out and had issues because the transitive sources here don't include the normal sources. I think transitive should be direct + indirect sources? In my case this causes issues when this is read by collect_es6_sources which only reads transitive_es6_sources...

Thanks! I didn't test it in a production bundle, that's missing here...

cgrushko · 2018-05-04T14:21:49Z

My 2 cents - ts_proto_library should be implemented in terms of a Bazel Aspect (I scanned the code and didn't see anything about it).
There are some details in https://blog.bazel.build/2017/02/27/protocol-buffers.html (look for "Bazel Aspects").
If this is already using Aspects, sorry, please disregard :)

alexeagle · 2018-05-04T14:37:19Z

Carmi, what would such an aspect do? Is there actually an action to cache at each proto_library node in the graph? In this case, the proto compiler does a global compilation over all the transitive sources so I don't think it would be an improvement, and adds complexity.

…

On Fri, May 4, 2018 at 7:21 AM Carmi Grushko ***@***.***> wrote: My 2 cents - ts_proto_library should be implemented in terms of a Bazel Aspect (I scanned the code and didn't see anything about it). There are some details in https://blog.bazel.build/2017/02/27/protocol-buffers.html (look for "Bazel Aspects"). If this is already using Aspects, sorry, please disregard :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#193 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC5I-OECUywVWwjTKnsmA6miH85B1W8ks5tvGP-gaJpZM4Tq_mQ> .

cgrushko · 2018-05-04T15:33:34Z

For all languages, protoc reads all transitive sources, and this step is unavoidable (due to protoc architecture) but also super cheap. OTOH, compiling the TS files themselves could he expensive. Java and C++ are expensive enough to be worth it. Remember that the number of compiled files is quadratic in the depth of the dependency tree. The bigger problem is ODR, though - without aspects you will end up with the same symbols generated by multiple rules. Depending on the language this could lead to linking errors (e.g. C++) or output file conflicts if the name of the output is derived from the names of the .proto files. Aspects also allow parallelism when compiling the generated files, vs transitive compilation.

…

On Fri, May 4, 2018, 10:37 Alex Eagle ***@***.***> wrote: Carmi, what would such an aspect do? Is there actually an action to cache at each proto_library node in the graph? In this case, the proto compiler does a global compilation over all the transitive sources so I don't think it would be an improvement, and adds complexity. On Fri, May 4, 2018 at 7:21 AM Carmi Grushko ***@***.***> wrote: > My 2 cents - ts_proto_library should be implemented in terms of a Bazel > Aspect (I scanned the code and didn't see anything about it). > There are some details in > https://blog.bazel.build/2017/02/27/protocol-buffers.html (look for > "Bazel Aspects"). > If this is already using Aspects, sorry, please disregard :) > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < #193 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAC5I-OECUywVWwjTKnsmA6miH85B1W8ks5tvGP-gaJpZM4Tq_mQ > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#193 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB5_YYVnb6lsxQoAp-bWr3S__cD4LNd3ks5tvGehgaJpZM4Tq_mQ> .

alexeagle requested review from gregmagolan and mrmeku April 27, 2018 20:48

googlebot added the cla: yes label Apr 27, 2018

gregmagolan approved these changes Apr 27, 2018

View reviewed changes

alexeagle force-pushed the issue61 branch from 7cb1370 to e703844 Compare April 30, 2018 22:23

Add ts_proto_library rule.

1e0de5e

Add example that uses it under a karma test. Fixes bazelbuild#61

alexeagle force-pushed the issue61 branch from e703844 to 1e0de5e Compare April 30, 2018 22:27

mrmeku reviewed Apr 30, 2018

View reviewed changes

Get AMD shims from the js_library in bootstrap[]

543aeba

gregmagolan reviewed May 1, 2018

View reviewed changes

Add ts_devserver bootstrap attribute for UMD scripts that must come b…

41f499c

…efore requirejs

alexeagle force-pushed the issue61 branch from 5ec3ee6 to 41f499c Compare May 1, 2018 17:38

Add test case for Long handling in int64

2828b3c

jbedard reviewed May 3, 2018

View reviewed changes

alexeagle closed this in eeb4eb9 May 7, 2018

easyCZ mentioned this pull request May 27, 2018

Add ts-protoc-gen-library rule to generate protos using Bazel improbable-eng/ts-protoc-gen#77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ts_proto_library rule. #193

Add ts_proto_library rule. #193

alexeagle commented Apr 27, 2018

gregmagolan commented Apr 27, 2018

gregmagolan left a comment

alexeagle commented Apr 30, 2018

mrmeku left a comment

mrmeku Apr 30, 2018

alexeagle May 1, 2018

mrmeku Apr 30, 2018

alexeagle May 1, 2018

cgrushko commented Apr 30, 2018

alexeagle commented May 1, 2018

gregmagolan May 1, 2018

jbedard May 3, 2018 •

edited

Loading

alexeagle May 3, 2018

cgrushko commented May 4, 2018

alexeagle commented May 4, 2018 via email

cgrushko commented May 4, 2018 via email

Add ts_proto_library rule. #193

Add ts_proto_library rule. #193

Conversation

alexeagle commented Apr 27, 2018

gregmagolan commented Apr 27, 2018

gregmagolan left a comment

Choose a reason for hiding this comment

alexeagle commented Apr 30, 2018

mrmeku left a comment

Choose a reason for hiding this comment

mrmeku Apr 30, 2018

Choose a reason for hiding this comment

alexeagle May 1, 2018

Choose a reason for hiding this comment

mrmeku Apr 30, 2018

Choose a reason for hiding this comment

alexeagle May 1, 2018

Choose a reason for hiding this comment

cgrushko commented Apr 30, 2018

alexeagle commented May 1, 2018

gregmagolan May 1, 2018

Choose a reason for hiding this comment

jbedard May 3, 2018 • edited Loading

Choose a reason for hiding this comment

alexeagle May 3, 2018

Choose a reason for hiding this comment

cgrushko commented May 4, 2018

alexeagle commented May 4, 2018 via email

cgrushko commented May 4, 2018 via email

jbedard May 3, 2018 •

edited

Loading