-
Notifications
You must be signed in to change notification settings - Fork 93
Fixed: use grpc-tools protoc to restore --js_out on modern protoc #714
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
base: main
Are you sure you want to change the base?
Fixed: use grpc-tools protoc to restore --js_out on modern protoc #714
Conversation
… protoc Signed-off-by: Constantin Chirila <[email protected]>
Signed-off-by: Constantin Chirila <[email protected]>
@WhitWaldo when you got the time to have a look at this. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 6 6
Branches 1 1
=========================================
Hits 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some notes about some protos to remove down the road, but it sounds from the comments that there are inappropriate imports coming from them.
Generally, only appcallback.proto, dapr.proto and common.proto should be referenced by the SDK. All the others are protos specific to the Dapr runtime internally and subject to change at any point. They should not be relied upon for type support.
@@ -131,6 +136,8 @@ downloadFile "https://raw.githubusercontent.com/$ORG_NAME/$REPO_NAME/$BRANCH_NAM | |||
downloadFile "https://raw.githubusercontent.com/$ORG_NAME/$REPO_NAME/$BRANCH_NAME/dapr/proto/internals/v1/apiversion.proto" "$PATH_ROOT/src/proto/dapr/proto/internals/v1/apiversion.proto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove all of these except those actually used by the SDK:
dapr.proto
common.proto
appcallback.proto
@@ -147,6 +154,8 @@ echo "" | |||
echo "Compiling gRPC files" | |||
generateGrpc "$PATH_ROOT/src/proto" "dapr/proto/common/v1/common.proto" | |||
generateGrpc "$PATH_ROOT/src/proto" "dapr/proto/internals/v1/apiversion.proto" | |||
# Also generate code for reminders to satisfy imports | |||
generateGrpc "$PATH_ROOT/src/proto" "dapr/proto/internals/v1/reminders.proto" | |||
generateGrpc "$PATH_ROOT/src/proto" "dapr/proto/internals/v1/service_invocation.proto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - we shouldn't actually be using any of these except appcallback.proto, dapr.proto and common.proto as they're internal to the runtime and subject to change at any point.
This looks great @ConstantinChirila - ping me when you've got the tests passing and I can merge this and your other PR and get an RC out the door for this SDK. |
Description
Problem:
Running
scripts/fetch-proto.sh
failed with:--js_out: protoc-gen-js: program not found or is not executable
On modern protoc versions, the builtin JS generator is no longer available the same way, so
--js_out
tries to invoke an externalprotoc-gen-js
that isn’t in PATH.This is due protoc v28 no longer providing the legacy/bundled JS generator, so
--js_out
requires a plugin discovery step that fails in current environment.grpc-tools
ships aprotoc
binary and plugins known to work with Node’s JS/TS generation this Eliminates PATH/plugin discovery issues.The change:
node_modules/.bin/grpc_tools_node_protoc
for generation; fall back toprotoc
if unavailable.--plugin=protoc-gen-ts=.../protoc-gen-ts
and--plugin=protoc-gen-grpc=.../grpc_tools_node_protoc_plugin
Issue reference
No issue reference. Needed to unblock further work.
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: