Empty array causes "Object not extensible"

[fathyb]

protobuf.js version: 6.0.0

When decoding messages with empty arrays or initializing a blank message with new(), all arrays are non extensible and inherited from the builder prototype.
It makes sense to freeze those array on the prototype, but when using new() or decode() we should have a working object, not frozen.

Can't we have usable objects without duplicating with Builder.from(message.toJSON()) and setting util.toJSONOptions.defaults to true?

Basically this shouldn't throw any error:

var message = new TestMessage({testList: []})
var buffer = TestMessage.encode(message).finish()
var decoded = TestMessage.decode(buffer)

// decoded.testList is undefined so it falls back to TestMessage.prototype.testList which is frozen       
decoded.testList.push('test str')

Replace {testList: []} with {testList: ['something']} and everything works back again.

I made a small repro repo to demonstrate what's going on.
Just run make and it should install everything and launch mocha. The first three tests should pass, the last two should fail.

[dcodeIO]

.decode adds own enumerable properties only if actually present on the wire, which allows the use of .hasOwnProperty to detect whether a field is actually present. For consistency this also includes repeated fields.

Besides providing a means of wire presence, this also reduces object initialization cost because everything not present just falls back to the prototype.

I know that this behavior isn't ideal, but unfortunately I don't see an ideal alternative either, so I decided to go with the most performant one. Internally, the library uses the following pattern to handle this case:

if (message.someArray.length) {
  message.someArray.push(...);
} else {
  message.someArray = [ ... ];
}

Earlier, I also tried working around this by implementing a getter on the prototype, that returns and replaces itself with a new empty array when invoked. Something like this:

Object.defineProperty(messagePrototype, "someArray", {
  get: function() {
    return this.someArray = [];
  },
  set: function(value) {
    delete this.someArray;
    this.someArray = value;
  }
});

but I wasn't sure about the performance impact this might have (i.e. regarding hidden class optimizations), so I didn't stick to it. If you have a better idea, let me know!

[fathyb]

I see, maybe we should provide a way for the user to choose, like it's done with toJSONOptions?
It can be done on the compiler or the runtime, performance wise the compiler would be the best solution.
This could be set using protobufjs/util or with a protobuf option like this:

option js_default_values = true; // default to false

When this value is to true the compiler would set the default values even if the wire is not present, and use this kind of code to detect wire presence.

const checkWire = (message, prop) =>
    message.hasOwnProperty(prop) &&
    (!Array.isArray(message[prop]) || message[prop].length !== 0)

checkWire({a: []}, 'a') // false
checkWire({a: ['b']}, 'a') // true
checkWire({a: 'b'}, 'a') // true
checkWire({}, 'a') // false

This behavior should be applied to new(), .fromObject/from() and .decode().
I don't think the performance cost will be high, .decode() will be slower because of array initialization, .encode() may be slower because of Array.isArray() and Array.length, but not significantly.

I'd be glad to work on a PR if you think it may be a good solution.

[dcodeIO]

I see, maybe we should provide a way for the user to choose, like it's done with toJSONOptions?

For toObject. there is arrays: true which initializes empty arrays.

[fathyb]

Yes I know but this doesn't apply to .decode() or new().

[dcodeIO]

For reference, this'd be a possible utility function to achieve it:

function populateArraysAndObjects(type, instance) {
  for (var i = 0; i < /* initializes */ type.fieldsArray.length; ++i) {
    var field = type._fieldsArray[i];
    if (instance.hasOwnProperty(field.name))
      return;
    if (field.map)
      instance[field.name] = {};
    else if (field.repeated)
      instance[field.name] = [];
  };
}

While that's quite simple and solely reflection based, it becomes a bit more complicated when codegen comes into play.

[fathyb]

I use static-module so I cannot use reflection :/. I really need this feature so I started to patch the codegen to optionally set defaults values when using .decode(), .encode() and new() with a --defaults switch and it appears to works.

It only supports non-packed repeated fields. I will update it soon to behave mostly like .toObject() w/ defaults: true.

If you think this could be integrated here I'll open a PR.

[dcodeIO]

Like encode and decode, constructor functions are now fully generated by Class.generate, which is also used by the static target from now on. There is already some commented out code for initializing object/array type fields as a starting point.

[fathyb]

That's great! I rebased my fork and used your commented out code only when --defaults is used, so the tests still passes. I'm gonna open a PR so you can start reviewing this.

[dcodeIO]

Found a way to integrate this without significantly breaking the API / any test cases. Let me know if this solves your issue.

[fathyb]

Thank you, this is exactly what I was looking for, all my tests passes now.

Are you publishing nightly/dev packages to npm and if not when will 6.7 be released?