sylvainpolletvillard / ObjectModel

Strong Dynamically Typed Object Modeling for JavaScript
http://objectmodel.js.org
MIT License
467 stars 30 forks source link

Optional array model cause duplication in other instances of a model with default #130

Closed BenjaminMINK closed 3 years ago

BenjaminMINK commented 4 years ago

Hello,

I've found a potential issue using an optionnal array model using push method when a default is set.

Accordling this script:

const Person = ObjectModel({
  name: String,
  female: Boolean
})

const Mother = Person.extend({
  female: true,
  child: [Person]
})

const Father = Person.extend({
  female: false,
  child: [Person]
})

const Family = ObjectModel({
  father: Father,
  mother: Mother,
  children: [ArrayModel(Person)],
  grandparents: [ArrayModel([Mother, Father])]
}).defaultTo({
  children: []
})

const joe = new Person({ name: 'Joe', female: false })
const ann = new Person({ name: 'Ann', female: true })
const marie = new Person({ name: 'Marie', female: true })
const joanna = new Person({ name: 'Joanna', female: true })

const joefamily = new Family({
  father: joe,
  mother: ann
})

const joefamily2 = new Family({
  father: joe,
  mother: marie
})

joefamily.children.push(joanna)

console.log(joefamily, joefamily2)

The result is, both Family instances have the same children:

Model {
  children: [ Model { name: 'Joanna', female: true } ],
  father: Model { name: 'Joe', female: false },
  mother: Model { name: 'Ann', female: true }
}
Model {
  children: [ Model { name: 'Joanna', female: true } ],
  father: Model { name: 'Joe', female: false },
  mother: Model { name: 'Marie', female: true }
}

Is that expected cause of the default? Or have I misunderstand something?

A workaround to fix that is to reset to array the children property: family.children = [] or to not use default on array models.

I'm not sure if other types do the same things...

sylvainpolletvillard commented 4 years ago

Hello,

Yes that's a recurrent confusion that we tried to mitigate in several ways in the past, see https://github.com/sylvainpolletvillard/ObjectModel/issues/93 for context for example

The problem is that the intention of the developer is unclear whether the default value should be a common reference or not. Depending on the example, sometimes it makes sense and sometimes not.

For example if we slightly change the example:

const referenceFamilyChildren = []; // might be updated later
Family.defaultTo({ children: referenceFamilyChildren })

this is technically the same yet it has a different meaning, and you would expect all Families without declared children would get back to this same reference.

In previous versions of ObjectModel, defaults were handled by using prototypal inheritance, so every instance of the model had the same reference to its default prop value defined in its prototype. The reference was common by purpose. But this pattern was found quite confusing for beginners with prototypes, so in v4 instead of using the prototype we do a deep merge of passed props with defaults. ie.

new Family({ children })this.children = deepmerge(children, model.default.children)

It solves many confusions when using default nested object properties, yet it does not change the fact that people tend to confuse defaults with initial values, which are two very different things. The empty array example is a classic one.

The correct way to do what you want would be to use a factory function to init your model:

const createFamily = ({ father, mother, children = [] }) => new Family({ father, mother, children })

since the default empty array is local to the function scope, each instance will have its own array.

BenjaminMINK commented 3 years ago

Hello,

Sorry for the response time, and thank you for this detailed answer!

Unfortunately, I don't know if I can help you on the future choice on how to manager/rewrite this in this library like you've said in a previous comment, sorry..!

sylvainpolletvillard commented 3 years ago

No problem. For now I will continue to remind the difference between defaults and initial values