Skip to content

Strip out _virtual_imports packages to support well known types #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Strip out _virtual_imports packages to support well known types #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3aa686
Strip out _virtual_imports packages to support well known types
Nov 18, 2019
cc5a319
Add wkt timetamp to test protos
Nov 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/typescript_proto_library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,13 @@ def _proto_path(proto):
path = path[len(root):]
if path.startswith("/"):
path = path[1:]
if path.startswith(ws):
if path.startswith(ws) and len(ws) > 0:
path = path[len(ws):]
if path.startswith("/"):
path = path[1:]
if path.startswith("_virtual_imports/"):
path = path.split("/")[2:]
path = "/".join(path)
return path

def _get_protoc_inputs(target, ctx):
Expand All @@ -53,7 +56,7 @@ def _build_protoc_command(target, ctx):

protoc_command += " --plugin=protoc-gen-ts=%s" % (ctx.executable._ts_protoc_gen.path)

protoc_output_dir = ctx.var["BINDIR"]
protoc_output_dir = ctx.bin_dir.path + "/" + ctx.label.workspace_root
protoc_command += " --ts_out=service=grpc-web:%s" % (protoc_output_dir)
protoc_command += " --js_out=import_style=commonjs,binary:%s" % (protoc_output_dir)

Expand Down Expand Up @@ -97,7 +100,13 @@ def _get_outputs(target, ctx):
js_outputs_es6 = []
dts_outputs = []
for src in target[ProtoInfo].direct_sources:
file_name = src.basename[:-len(src.extension) - 1]
# workspace_root is empty for our local workspace, or external/other_workspace
# for @other_workspace//
if ctx.label.workspace_root == "":
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment describing when workspace_root is empty?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

file_name = src.basename[:-len(src.extension) - 1]
else:
file_name = _proto_path(src)[:-len(src.extension) - 1]

for f in ["_pb", "_pb_service"]:
full_name = file_name + f
output = ctx.actions.declare_file(full_name + ".js")
Expand Down
2 changes: 2 additions & 0 deletions test/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ ts_library(
deps = [
"//test/proto:pizza_service_ts_proto",
"//test/proto/common:delivery_person_ts_proto",
"@npm//google-protobuf",
"@npm//@types/google-protobuf",
"@npm//@types/jasmine",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to change? I'd prefer to be as specific as possible so it's easier to trim unused dependencies.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

],
)
Expand Down
6 changes: 5 additions & 1 deletion test/commonjs_test.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import deliveryPersonPb = require('rules_typescript_proto/test/proto/common/delivery_person_pb');
import {Timestamp} from 'google-protobuf/google/protobuf/timestamp_pb';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This proto should be at <workspace-name>/path/to/proto, so it should be: com_google_protobuf/google/protobuf/timestamp_pb

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's the case here. This syntax would load from the npm module google-protobuf rather than the user's own workspace

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to say that the google-protobuf version has special code for timestamp conversion and stuff, but then went and researched and the protoc compiler itself adds that in here https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/compiler/js/well_known_types_embed.cc

Dig-Doug: do you still want this changed to use the non-npm locally generated proto code here?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My impression here was that you're trying to test if you can generate a TS protobuf for a proto defined in an external repository.

E.g., let's say you wanted to use one of the protos defined in the rules_typescript_proto repo in your own project. For that case, I'd expect to reference the proto as rules_typescript_proto/path/to/proto. So for the code above, you could use protos in the protobuf repo to simulate the case I described above.

Alternatively, you could also handle google-protobuf as a special case and do something different. However, I don't see a reason not to use the locally generated versions. I don't think the WKTs are included in the main google-protobuf.js bundle, so there won't be any code duplication. I also think you would need to do some additional work to serve those other WKT protos from the npm library in ts_devserver/bundling that you would not need with files generated by bazel.

For those reasons, I'm leaning towards using the locally generated files. I don't have a strong opinion here, so if there's a reason we should be using the bundled versions instead let me know.

import * as deliveryPersonPb from 'rules_typescript_proto/test/proto/common/delivery_person_pb';
import {PizzaService} from 'rules_typescript_proto/test/proto/pizza_service_pb_service';

describe('CommonJs', () => {
Expand All @@ -7,6 +8,9 @@ describe('CommonJs', () => {

const person = new deliveryPersonPb.DeliveryPerson();
person.setName('Doug');
const lastDeliveryTime = new Timestamp();
lastDeliveryTime.fromDate(new Date());
person.setLastDeliveryTime(lastDeliveryTime);
expect(person).toBeDefined();
});

Expand Down
1 change: 1 addition & 0 deletions test/proto/common/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ proto_library(
"delivery_person.proto",
],
deps = [
"@com_google_protobuf//:timestamp_proto",
":pizza_proto",
],
)
Expand Down
3 changes: 3 additions & 0 deletions test/proto/common/delivery_person.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ syntax = "proto3";

package test.bazel.proto;

import "google/protobuf/timestamp.proto";
import "test/proto/common/pizza.proto";

message DeliveryPerson {
Expand All @@ -16,4 +17,6 @@ message DeliveryPerson {
string id = 3;
int64 id_v2 = 4;
}

google.protobuf.Timestamp last_delivery_time = 5;
}
10 changes: 9 additions & 1 deletion test/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@
"noEmitOnError": true,
"types": [
"jasmine"
]
],
"baseUrl": "..",
"paths": {
"rules_typescript_proto/*": [
"*",
"bazel-bin/*"
]
}

}
}