Definition of nested list models

[gibelium]

First of all thank you for this awesome library. We use it within meteor and are pretty happy.

Today I came across the definition and default initialization of nested list-models (e.g. ArrayModel and SetModel). Defining the property itself is straight forward, but defining a mandatory list-model to be empty by default took me some time and feels a little bit unhandy or let's say I assumed to be different. I used version 4 from the examples default initialization and wondered why it does not work as it does not throw an error during runtime.

Please find a simple codepen example underneath.

My first question is if you can imagine to simplify the definition of a list models default value for instance with a basic javascript array syntax. This at least could be "[]" defining a empty array/set. Perhaps this syntax can also be used for initializing a list model with actual default values.

My second question came up while creating the example. Why does version 2 work with and without the new operator?

Thank you and regards, Sebastian

Example Code

class NestedVO extends ObjectModel({
  numberProperty: Number,
}).defaults({
  numberProperty: 0,
}) {
  constructor(modelDefinition) {
    super(modelDefinition);
  }
}
const NestedVOSetModel = SetModel(NestedVO);
//NestedVOSetModel.add(new NestedVO({ numberProperty: 1 }))
console.log(NestedVOSetModel);
const NestedStringsSetModel = SetModel(String);
class VO extends ObjectModel({
  stringProperty: String,
  nestedStringsSetModel: NestedStringsSetModel,
  nestedVOsSetModel: NestedVOSetModel ,
  nestedStringsArrayModel: ArrayModel(String),
  nestedVOsArrayModel: ArrayModel(NestedVO),
}).defaults({
    // 1) explicitly predefined Model const
  nestedStringsSetModel: NestedStringsSetModel( [] ), 

    // 2) works with or without new (also true for 1) 
  nestedVOsSetModel: new NestedVOSetModel( [] ), 

    // 3) most concise working syntax
  nestedStringsArrayModel: ArrayModel(String)([]), 

    // 4) doesn't work, but wishing it would ; - )
  nestedVOsArrayModel: [], 
}) {
  constructor(modelDefinition) {
    super(modelDefinition);
  }
}

let normalVO = new VO({ stringProperty: 'Test String' });
console.log('normalVO.stringProperty: ' + normalVO.stringProperty);

// Add String properties to SetModel
console.log(
  'Size of String SetModel (before): ' + normalVO.nestedStringsSetModel.size
);
normalVO.nestedStringsSetModel.add('first ');
normalVO.nestedStringsSetModel.add('second');
normalVO.nestedStringsSetModel.add('third');
console.log(
  'Size of String SetModel (after): ' + normalVO.nestedStringsSetModel.size
);

// Add String properties to ArrayModel
console.log(
  'Size of String ArrayModel (before): ' + normalVO.nestedStringsArrayModel.length
);
normalVO.nestedStringsArrayModel.push('first');
normalVO.nestedStringsArrayModel.push('second');
normalVO.nestedStringsArrayModel.push('third');
console.log(
  'Size of String ArrayModel (after): ' + normalVO.nestedStringsArrayModel.length
);

// Add nested objects to SetModel
console.log(
  'Size of NestedVO SetModel (before): ' + normalVO.nestedVOsSetModel.size
);
normalVO.nestedVOsSetModel.add(new NestedVO({ numberProperty: 1 }));
normalVO.nestedVOsSetModel.add(new NestedVO({ numberProperty: 2 }));
normalVO.nestedVOsSetModel.add(new NestedVO({ numberProperty: 3 }));
console.log(
  'Size of NestedVO SetModel (after): ' + normalVO.nestedVOsSetModel.size
);

// Add nested objects to ArrayModel
console.log(
  'Size of NestedVO ArrayModel (before): ' + normalVO.nestedVOsArrayModel.length
);
normalVO.nestedVOsArrayModel.push(new NestedVO({ numberProperty: 1 }));
normalVO.nestedVOsArrayModel.push(new NestedVO({ numberProperty: 2 }));
normalVO.nestedVOsArrayModel.push(new NestedVO({ numberProperty: 3 }));
console.log(
  'Size of NestedVO ArrayModel (after): ' + normalVO.nestedVOsArrayModel.length
);

[gibelium]

In a breakpoint on the second normalVO.nestedVOsArrayModel.push command (near the bottom at line 72), one can see that normalVO.nestedVOsArrayModel IS a Proxy of an Array instance with length 0. One would expect these push statements to function:

[sylvainpolletvillard]

Hi, thanks for the detailed issue. I am currently on vacation and will check this out next week.

[sylvainpolletvillard]

First question: there is a classic value/reference mistake, although I admit I don't understand why the array is not mutated. Probably a bug, needs investigation.

The defaults method is just a utility method to quickly assign properties to the model prototype. So:

myObjectModel.defaults({ prop: [] })

is equivalent to write

myObjectModel.prototype.prop = []

That means that by specifying an empty array in the model prototype, all the model instances will point to the same array reference, which is not what you want. Instead, you want every new instance to have its own new array.

Usually, developers solve this problem by assigning a new array in the class constructor. Because the array is literally declared in the constructor code, each instance gets a fresh new array.

Maybe there is something we could do on the library side to make this easier for the developer. I am not fully satisfied with the current way of assigning default values to model instances, with the weird defaults/defaultTo distinction. This is not a trivial problem, but I think we can do better.

In the meantime, the best you can do is to avoid putting mutable objects in model defaults, and instead declare them in the constructor. This is not an issue for all the immutable defaults like numbers, booleans, strings...

[sylvainpolletvillard]

Second question is easier to answer: I added additional checks in model and model instances constructors to ensure that the developper did not forget the new operator, so you can just stop thinking about it.

I personally think the new operator is fundamentally broken in JavaScript, and will do everything I can to avoid people to have to deal with its quirks

[gibelium]

Thank you for your quick investigation.

I now ended up with a private factory function that is called by the constructor and creating the new instance of the array/set/map models there, which is most elegant for mandatory list models from my point of view.

[sylvainpolletvillard]

I also think it is the most elegant solution currently available. I keep in mind that defaults assignment needs a rework for future versions of ObjectModel. Other libraries separate data and methods to differ between prototype assignment and constructor assignment, but I don't really like this pattern. Sometimes you legit need to put some values in the prototype or at the contrary have some functions not in the prototype. Another option would be to dynamically assign everything and let the user manually put stuff in the prototype of he knows what he is doing. There is a compromise to find between simplicity vs convenience.

[sylvainpolletvillard]

I started to work on the default assignment rework, feel free to comment or make suggestions here: #93

[sylvainpolletvillard]

I admit I don't understand why the array is not mutated. Probably a bug, needs investigation.

Bug found, it was because provided defaults were not autocasted. Before v3.7.3, you had to manually cast any default value to their suitable model, as you did in 3)

This is fixed in v3.7.3, where default values are checked against definition and autocasted.

@gibelium Could you upgrade and confirm that the problem is solved ?

[gibelium]

Thank you for fixing this. I can confirm that the issue is solved with ObjectModel 3.7.3.