-
Notifications
You must be signed in to change notification settings - Fork 0
Support @bufbuild/protobuf
#1
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
Conversation
Signed-off-by: Sri Krishna <[email protected]>
srikrsna-buf
left a comment
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.
Figuring out the tests
| bool grpc_js; | ||
| // Omit instanceof check for messages in serialize methods | ||
| bool omit_serialize_instanceof; | ||
| // Runtime to use for protobuf serialization (default: "google-protobuf", "es" for @bufbuild/protobuf) |
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.
Perhaps we should use bufbuild-protobuf instead of es
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.
Would be a better fit for the "runtime" argument. I wouldn't change the example directory name, and it isn't feasible to change --es_out.
| bool grpc_js; | ||
| // Omit instanceof check for messages in serialize methods | ||
| bool omit_serialize_instanceof; | ||
| // Runtime to use for protobuf serialization (default: "google-protobuf", "es" for @bufbuild/protobuf) |
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.
Would be a better fit for the "runtime" argument. I wouldn't change the example directory name, and it isn't feasible to change --es_out.
| } | ||
|
|
||
| grpc::string NodeObjectPath(const Descriptor* descriptor) { | ||
| grpc::string NodeObjectPath(const Descriptor* descriptor, const grpc::string& runtime) { |
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.
Don't know how to test it, but the diff looks good to me.
Fix example
Signed-off-by: Sri Krishna <[email protected]>
Signed-off-by: Sri Krishna <[email protected]>
Add support for
@bufbuild/protobufruntimeThis PR adds support for generating service definitions compatible with the
@bufbuild/protobufruntime, a popular and conformant protobuf implementation.What this PR does:
Adds a new plugin option called
runtimeto select the protobuf runtime. Supported values arebufbuild-protobufand defaults to usegoogle-protobuf. When the runtime is set tobufbuild-protobuf, the plugin generates service definitions that use@bufbuild/protobuf's message types and serialization methods instead of the default runtime.Why this change?
@bufbuild/protobufsupports Editions and passes significantly more conformance tests than currently supported runtimes, ensuring better protocol buffer compatibility.Usage
Alongside
grpc-tools, users will need to install@bufbuild/protoc-gen-esand change thegrpc_tools_node_protoccall to use the new runtime and use theprotoc-gen-es:Alternatives
Happy to discuss other options like a separate runtime package or plugin package. Having either of those will also bring TypeScript and ESM support.