stephenh / ts-proto

An idiomatic protobuf generator for TypeScript
Apache License 2.0
2.17k stars 349 forks source link

Support proto2 enums w/o a zero value #663

Closed jung-kim closed 1 year ago

jung-kim commented 2 years ago

portobuf: https://github.com/apache/pulsar/blob/a73e1f3a2d54d626fc1dfe01e55bc1b8ab0ee8f2/pulsar-common/src/main/proto/PulsarApi.proto command: protoc --plugin=node_modules/ts-proto/protoc-gen-ts_proto ./src/proto/PulsarApi.proto -I. --ts_proto_out=. --ts_proto_opt=esModuleInterop=true

Using above command, I'm not sure why the generated marshal code conditionally writes a required value type if type is 2, which is CommandConnect type. Looking a the protobuf file, I'm not sure why this is as CommandConnect type doesn't require nor specify such treatment. Similarly generated golang code doesn't have such issue.

stephenh commented 2 years ago

Hey @jung-kim ; sorry for the late response, but yeah, I'm pretty sure this issue is due to this line of code:

https://github.com/stephenh/ts-proto/blob/main/src/types.ts#L192

Which was originally written with the proto3 assumption that enums require a value with 0; and, when it was written, it was simplest to say "if there is not a 0 value enum, use the first one".

However, you're using proto2, which doesn't have this restriction.

It should be possible to change this line in the code to, instead of picking the first value, just return undefined, i.e. indicate there is no appropriate default value for this field.

This would be really easy, but then the 2nd step would be to update any callers of defaultValue to handle it returning undefined.

I won't personally have time to work on this, but if you'd like to work on a PR for this, that'd be great!

stephenh commented 2 years ago

(We may actually have an existing issue for this, b/c someone else has brought up the "proto2 enums w/o a zero value" before...for now just re-titling this issue.)

stephenh commented 1 year ago

We have usePrototypeForDefaults now which supports this.