fix: make optional values optional

[achingbrain]

If we have values marked optional like this:

message MyThing {
  optional int32 value = 1;
}

Decoding a buffer where value was not set defaults it to 0:

const buf = MyThing.encode({})
const myThing = MyThing.decode(buf)

console.info(myThing.value)
// prints '0'

This PR changes the decoding so myThing will not have a value property so optionals actually become optional and we can differentiate between the case where value was explicitly set to 0 and where it was not set at all:

const buf = MyThing.encode({})
const myThing = MyThing.decode(buf)

console.info(myThing.value)
// prints 'undefined'

console.info(Object.keys(myThing).includes('value'))
// prints 'false'

The change has been made for strings, booleans, bytes, custom types, all number types and tests have been added where they were missing.

[alanshaw]

This PR changes the decoding so myThing.value will be returned with the value null so optionals actually become optional and we can differentiate between the case where value was explicitly set to 0 and where it was not set at all.

How come null and not undefined? (i.e. not set to undefined, actually, really undefined). Would it be better to round trip and get the same object back?:

const buf = MyThing.encode({})
const myThing = MyThing.decode(buf)

console.info(myThing) // {}, not { value: null }

[achingbrain]

How come null and not undefined?

Good shout, have made that change. I don't think protobufs even support null as a value (protocolbuffers/protobuf#1606) so this makes more sense.

[achingbrain]

Hmm, I think this returning 0 for non-specified optional numeric types is considered a feature:

When a message is parsed, if it does not contain an optional element, the corresponding field in the parsed object is set to the default value for that field.

The default value can be specified as part of the message description.

If the default value is not specified for an optional element, a type-specific default value is used instead: for strings, the default value is the empty string. For bytes, the default value is the empty byte string. For bools, the default value is false. For numeric types, the default value is zero. For enums, the default value is the first value listed in the enum's type definition.

https://developers.google.com/protocol-buffers/docs/proto#optional

[alanshaw]

So, we close this? I did wonder if it didn't do this already for a reason...

[achingbrain]

I think we have to, unless we want to have our own interpretation of protocol buffers that doesn't follow the spec 100% (not the end of the world, if it allows us to support data models where the absence of a value is a valid value).

[achingbrain]

..so it turns out the protoc compiler for protocol buffers spits out hasXXX methods for optional properties when compiling .proto files to JavaScript, but only for v2 protobuf schemas so there is some scope for detecting whether fields have been set.

Given:

optional uint32 foo;

You'll get something like:

class Datatype {
  constructor () { ... some code here ... }

  hasFoo () {
    return this._foo !== undefined
  }

  getFoo () {
    if (this.hasFoo()) {
      return this._foo
    }

    return 0 // the 'zero value', https://tour.golang.org/basics/12
  }

  setFoo (foo) {
    this._foo = foo
  }

  clearFoo () {
    this._foo = undefined
  }
}

Do we want to support this sort of method creation? It seems un-javascripty to me. I'd rather only define a property on the object if the value has been set so attempts to reference if when it's not been set will return undefined - that is how you'd detect if it's been set or not. Returning values where no value has been set doesn't seem terribly useful to me.

I've tried a few different protobuf libraries - protobufjs, pbf - the only one that seems to handle detecting if a field has been set is google-protobuf and it's via the hasXXX methods above.

google-protobuf is 10x the size of protons so I don't think switching to it is an option.

[achingbrain]

The go version puts values behind pointers and adds a getXXX method for each field, so you can either access the field directly and detect if it's been set, or call the getter and get the zero value if it's not set.

We can emulate similar behaviour by merging this PR, with the exception of the getter, which we can add if anyone asks for it.

[alanshaw]

If the accessor methods are added to this library we'll be able to determine if a field exists or not, and it can probably be released as a non-breaking change provided they're non-enumerable.

[achingbrain]

Superseded by #9