feat: generalize js_proto_toolchain() to work with other implementations#2743
feat: generalize js_proto_toolchain() to work with other implementations#2743jbedard merged 23 commits intoaspect-build:mainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
alexeagle
left a comment
There was a problem hiding this comment.
Did you confirm that other plugins actually make use of this new ability?
So far I have only confirmed that it works with protobuf-es and Google's protobuf-javascript implementation, so I still need to confirm that for stephenh/ts-proto and timostamm/protobuf-ts. |
5e69c0f to
b03fd17
Compare
|
@alexeagle I'd like to clean up and improve my examples a bit, but otherwise I think this PR is about ready for a closer review when you have a chance. I was able to set up an example for each of the JS/TS implementations that Buf provides an SDK for. Do you have any suggestions on the best way to make sure my examples run on CI? |
8ecae79 to
1855a64
Compare
|
is there a good reason not to just make these examples/ and part of the root module? then they are easier for users to discover and don't require maintaining nested modules or CI changes |
I think that should work. Once we have all four toolchains registered in the same module, I suspect we'll need a config transition to be able to choose the one we want for each example without requiring a command-line flag. Let me give that a try. |
666672e to
c5739ba
Compare
|
I think I now have something mostly working that allows all the examples to live in the same module. It still needs a bit of cleaning up, though. |
6815170 to
3f72c09
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f72c09ae8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
1b40bee to
8660865
Compare
|
@jbedard I think this is ready for another look whenever you have a chance. |
|
@jbedard I think this is now ready for another look. I'm not sure what the CI errors are about but maybe you have a better idea of what the problem is there? |
|
You can ignore the |
Currently we are not set up to support any JavaScript or TypeScript protobuf implementations other than protobuf-es from Buf.build. This is because we have a hard-coded assumption that the protobuf plugin will produce files matching the .proto file name except with "_pb.js" and "_pb.d.ts" in place of ".proto". This assumption holds for protobuf-es but not for other implementations such as Google's protobuf-javascript implementation. This change addresses that problem by adding a new `output_file_extensions` parameter on `js_proto_toolchain()` to allow users to specify the generated files they expect the plugin to produce. To make this work, I had to copy most of the logic from the `proto_lang_toolchain()` rule into a new `js_proto_toolchain()` rule. This was necessary because we need a rule that continues to produce the `ProtoLangToolchainInfo` provider but additionally produces a new `JsProtoToolchainInfo` provider with the new file extension information we need.
Currently we are not set up to support any JavaScript or TypeScript protobuf implementations other than protobuf-es from Buf.build. This is because we have a hard-coded assumption that the protobuf plugin will produce files matching the .proto file name except with "_pb.js" and "_pb.d.ts" in place of ".proto". This assumption holds for protobuf-es but not for other implementations such as Google's protobuf-javascript implementation.
This change addresses that problem by adding a new
output_file_extensionsparameter onjs_proto_toolchain()to allow users to specify the generated files they expect the plugin to produce.To make this work, I had to copy most of the logic from the
proto_lang_toolchain()rule into a newjs_proto_toolchain()rule. This was necessary because we need a rule that continues to produce theProtoLangToolchainInfoprovider but additionally produces a newJsProtoToolchainInfoprovider with the new file extension information we need.Changes are visible to end-users: yes
Test plan