Skip to content
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.

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

Consider aligning API w/ Bazel proto idioms #6

Closed
perezd opened this issue Feb 9, 2020 · 2 comments
Closed

Consider aligning API w/ Bazel proto idioms #6

perezd opened this issue Feb 9, 2020 · 2 comments
Labels
enhancement New feature or request

Comments

@perezd
Copy link

perezd commented Feb 9, 2020

Hey there, I was reviewing this example and I notice your example could be more idiomatic with existing Bazel proto rules.

  1. <lang>_proto_library should prefer to use depson a proto_library vs referring to it as a src. Example can be found here: https://docs.bazel.build/versions/master/be/java.html#java_proto_library. The rationale being this is generating source code from a collection of dependencies and is usually expressed that way.

  2. ts_library targets that wish to use the generated ts lang proto library should express dependency on the target using deps instead of src. It's considered generally a best practice to declare a label in srcs that is directly are directly consumed by the rule that output source files (citation). In this particular case, I believe the ts_library is depending on a previously generated target and wouldn't be expected to list it as a src of the library itself. Additionally, its considered a best practice to not have the same file listed in srcs of multiple targets, which this would cause.

@thesayyn
Copy link
Owner

Hey, I agree with the first one that says _proto_library should definitely pick up the descriptors from "deps" attribute instead of "srcs".

Since _proto_library is a source generating target, we should not think it as a JS-producing target and also I'm not sure if ts_library picks up source files from the "deps" attribute. (I guess I'm gonna see if it works that way.)

@thesayyn thesayyn added the enhancement New feature or request label Feb 13, 2020
@thesayyn
Copy link
Owner

Fixed in 0.0.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants