Strip out _virtual_imports packages to support well known types #11
Conversation
| @@ -97,7 +100,11 @@ 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] | |||
| if ctx.label.workspace_root == "": | |||
There was a problem hiding this comment.
Can you add a comment describing when workspace_root is empty?
|
I added a wkt to one of the protos and the commonjs test passes, but I'm getting a failure in the web test for which I couldn't figure out the right solution. |
There was a problem hiding this comment.
Re the 404 error: There is an issue with the path the timestamp proto is being referenced by, if you look at the delivery_person_pb.js file, you'll see:
var google_protobuf_timestamp_pb = require('google-protobuf/google/protobuf/timestamp_pb');This isn't the correct path (see my other comment). You can look at the code by either running the test or looking at the generated file:
vim bazel-bin/test/proto/common/delivery_person_pb.jsCan you see if you can update the implementation to use the workspace name in the path?
| @@ -24,7 +24,7 @@ ts_library( | |||
| deps = [ | |||
| "//test/proto:pizza_service_ts_proto", | |||
| "//test/proto/common:delivery_person_ts_proto", | |||
| "@npm//@types/jasmine", | |||
There was a problem hiding this comment.
Do these need to change? I'd prefer to be as specific as possible so it's easier to trim unused dependencies.
| @@ -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'; | |||
There was a problem hiding this comment.
This proto should be at <workspace-name>/path/to/proto, so it should be: com_google_protobuf/google/protobuf/timestamp_pb
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
I haven't tested extensively, but this seems to work for me after merging. gonzojive@41da6ce |
|
What's the status of this? I just realised that I can't easily dep on |
This is a first pass at working with the well known types and _virtual_imports. If this looks like a good approach, I can add some tests for it.
Fixes #4