protoc-gen-ng: Optional/Required fields when using proto2

[isc30]

Hi, I'm aiming to avoid all the typescript errors regarding "everything is optional" in proto3 by using proto2 (where you can specify optional and required).

It seems like the plugin (protoc-gen-ng) isn't generating the typings properly for those cases, and still emits string | undefined signature.

Any ideas how to fix this?

[isc30]

Seems like the || undefined bit is hardcoded in
https://github.com/ngx-grpc/ngx-grpc/blob/master/packages/protoc-gen-ng/src/output/types/fields/string-message-field.ts#L79

[isc30]

When using proto3 optional fields experimental feature (--experimental_allow_proto3_optional) this is the error message I get:

XXX.proto: is a proto3 file that contains optional fields, but code generator protoc-gen-ng hasn't been updated to support optional fields in proto3. Please ask the owner of this code generator to support proto3 optional

[smnbbrv]

Hi @isc30

• proto2 is not intendend to be supported; so everything is considered optional from proto3 prospective, as you correctly note. And I must admit it should not anyhow be reflected in TypeScript; even if the field is optional and is scalar it is still not-null / not-undefined
• I cannot name what was the original idea behind having everything optional. It might be the reason is already gone, since there were a lot of changes since then. However, what should always be optional is message fields or the members of oneof, that's just by Protobuf definition
• experimental_allow_proto3_optional is not supported ATM and I don't really know what's required on plugin side to support it. There is a library underneath protoc-gen-ng called protoc-plugin, it might be this lib needs to be adapted in order to support it.

It might make sense to remove the optional markers on the scalar fields. However, one still can assign them to undefined, can't one?

[isc30]

When defining optional fields, it marks the rest as required (proto2 style) and that's where TS should not contain the | undefined in this case. The gain isn't massive but it allows to get compiler errors instead of sending invalid data to the server

[isc30]

same issue here
protobufjs/protobuf.js#1468

[smnbbrv]

hi @isc30

the intention is clear; could you share your thoughts on how exactly you see this?

What should be changed? What is desired output for relevant parts of the protobuf file?

[isc30]

Hi, since proto2 seems to be working atm, i would get extra metadata on the fields to see if they were decorated as required or optional. Then, for required fields the hardcoded place can omit the | undefined bit.

I tried building the repo but not sure how to debug the change.

[smnbbrv]

Closing in favour of #54

Proto 2 is not going to receive a lot of support anyway