stephenh / ts-proto

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

Don't encode proto2 optional fields #973

Open Demon000 opened 12 months ago

Demon000 commented 12 months ago

Is this supported and I just can't find the correct flags to use, or is this unsupported?

stephenh commented 12 months ago

Hi @Demon000 , proto2 should be supported; it was not an initial goal of ts-proto, but I think it basically works at the moment.

The biggest flag for proto2 support is usePrototypeForDefaults because it disables the proto3 behavior of "always fill in missing keys with their default value". proto2 code would sometimes rely on the presence of "is this key set or what?", i.e. hazzer functions, and so usePrototypeForDefaults enables hazzer checks.

Otherwise, yeah, it should work. Let me know if it doesn't, and I can try and point you in the right direction of a fix/PR. Thanks!

Demon000 commented 12 months ago

Hi @Demon000 , proto2 should be supported; it was not an initial goal of ts-proto, but I think it basically works at the moment.

The biggest flag for proto2 support is usePrototypeForDefaults because it disables the proto3 behavior of "always fill in missing keys with their default value". proto2 code would sometimes rely on the presence of "is this key set or what?", i.e. hazzer functions, and so usePrototypeForDefaults enables hazzer checks.

Otherwise, yeah, it should work. Let me know if it doesn't, and I can try and point you in the right direction of a fix/PR. Thanks!

Thanks for the quick answer.

The first issue I encountered is that fields marked as optional would not translate to a field: Type | undefined.

The second issue I have is that fields with set default values will not be sent over the wire.

I'm not sure how usePrototypeForDefaults helps with these cases, it doesn't seem to.

stephenh commented 12 months ago

The second issue I have is that fields with set default values will not be sent over the wire.

Ah, okay, that makes sense. I think the only place I've personally used proto2 optional fields is reading them, and not putting them on the wire.

I think we'd need some sort of encodeDefaultValues flag, which hopefully shouldn't be that hard to support, if you're interested in submitting a PR, that'd be great!

stephenh commented 12 months ago

The first issue I encountered is that fields marked as optional would not translate to a field: Type | undefined.

I guess this makes sense...when ts-proto was first written, it was when proto3 didn't have optional fields. Then after a few years, proto3 added support for optional fields, as like oneofs, and so we support that ... but probably not the "quick & simple" approach of proto2 of just "do or do not put the value on the wire".

Fwiw personally I wish proto3 would have kept proto2's optional field semantics, instead of asserting for ~4-5 years that "no one needs optional fields", and then just backtracking on it later. :shrug:

Apologies for the late replies, I can't always immediately triage ts-proto issues, but try to check-in about once per week or two.

I think both of these issues you're seeing are fixable if you're interested in poking around, like the main.ts generateEncodeMethod iirc.

Demon000 commented 12 months ago

Sorry, I switched to protobuf-es which seems to handle my usecase correctly. I first tried to fix this project but the code is too all over the place for me.

stephenh commented 12 months ago

Ah yeah; I won't deny that--ts-proto's "strength" is that its collected a large array of knobs/flags over the years that, in theory, let users adapt the output to whatever their random/esoteric protobuf internal stacks, but yeah the complexity has gone up to support that. :shrug:

Not really a problem I'm paid to solve :-) , so happy you found protobuf-es does what you want.

stephenh commented 8 months ago

Fwiw @lukealvoeiro I was looking for any issues tagged with "proto2" to close out, after you landed #1007 ... seems like this one is still valid, i.e. an ask to recognize field values as "matching the optional value", and not encode them.

Which is really similar to what proto3 does with its default values, but probably still something we don't do, which is fine imo/not high priority, just noting my current understanding.