protobufjs / protobuf.js

Protocol Buffers for JavaScript & TypeScript.
Other
9.91k stars 1.41k forks source link

Implement Field Presence #1406

Open drungrin opened 4 years ago

drungrin commented 4 years ago

Protobuf release 3.12 adds experimental support for optional fields in proto3. Proto3 optional fields track presence like in proto2.

https://github.com/protocolbuffers/protobuf/blob/master/docs/field_presence.md

https://github.com/protocolbuffers/protobuf/blob/master/docs/implementing_proto3_presence.md

yinzara commented 3 years ago

We'd REALLY love if this was implemented :) optional fields is the biggest drawback we've had with implementing protobuf and without this implementation we have to use wrapper types which also don't have the nicest implementation either

kskalski commented 3 years ago

Note that currently the field presence kind of works already even though there is not special API or official support in the library. In my project I use optional fields extensively and the messages parsed by protobufjs seem to follow the semantics of field presence:

alexander-fenster commented 3 years ago

Hi folks, I added this feature in #1584, please feel free to npm install protobufjs@next and check if it works for you. To be released soon.

kskalski commented 3 years ago

After this change, is it expected that .toObject(from_pb, { defaults: true }) will not populate the optional fields that are not explicitly set on the from_pb message? I'm observing a change in semantics compared to previous library version... though it's not completely clear what the behavior should be.

In other languages unset optional field will still return the default value when accessed (through getter) and it has separate API for checking whether it is set or not. I suppose it is not the way it works in Javascript, where the implementation is using undefined for the unset fields. I think this is fine, though it would still be useful to provide a way to access the defaults as it's possible in other languages. In the previous version I relied on the way optional fields were treated:

I imagine the easiest way to provide both semantics to the user would be to introduce another option to toObject method, like optionalDefaults that would allow us to control which defaults are populated (alternatively bring back previous semantics of defualts: true populating all fields by making it imply optonalDefaults: true when it is not provided).

What are your thoughts? Or maybe protobufjs should follow instead the way other languages approach optonal API and create separate methods for checking presence?

kskalski commented 3 years ago

One more question - I noticed that for optional fields there are _fieldname: "fieldname" members generated in the API. Is this really necessary? I suppose the syntheric oneofs should be invisible to the end user, the proto3 docs seem to suggest it too: "Avoid generating any oneof-based accessors for the synthetic oneof. Its only purpose is to make reflection-based algorithms work properly if they are not aware of proto3 presence. The synthetic oneof should not appear anywhere in the generated API."

Addition of those fields brings a lot of clutter. They double the footprint of the objects with no apparent value, especially that they are kind of trivial constant value accessors.

alexander-fenster commented 3 years ago

The spec only asks not to generate accessors (getFoo, setFoo in other languages) which I believe we don't do anyway. I'm not sure if it's possible to hide those synthetic oneofs completely :(

alexander-fenster commented 3 years ago

As for the methods to check presence, this would obviously be a cleaner solution than the one we have now but I don't think I'll be able to find time to contribute it. PRs are welcome! :)