sylvainpolletvillard / ObjectModel

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

Model composition and defaults #163

Closed juanjoseruiz closed 1 year ago

juanjoseruiz commented 1 year ago

Hello,

I'm trying migrate a deployment from 2.x to latest version and I am running into some issues when using model composition and defining the defaults on each of them.

Given the following code:

const Person = new Model({
        name: String,
        phone: String,
    }).defaultTo({
        name: "Unknown name",
        phone: "Unknown phone"
});

const Register = new Model({
        id: String,
        writable: Boolean,
        person: Person
    }).defaultTo({
        id: "Untitled file",
        writable: true
});

const reg = new Register({ person: { name: 'Person name' }});

I was expecting the object reg to contain the default properties for person if not defined, but doesn't seem to be working in that way. Instead, I am getting an object of the correct model but only with the name set (the one defined as part of the input).

What it's strange, is that if I remove the defaults from Person and try to instantiate Register in the same way, I am getting an error because 'phone' was not defined, so it seems to be using the defaults somehow.

I also tried to use prototype inheritance with same result.

The only way I made it work was specifying the defaults of 'Person' in the 'Register' model itself like this:

const Register = new Model({
        id: String,
        writable: Boolean,
        person: Person
    }).defaultTo({
        id: "Untitled file",
        writable: true,
                person: {
            name: "Unknown name",
            phone: "Unknown phone"
                }
});

This same composition pattern was working fine using 2.x version. Is this an issue? Or am I doing something wrong?

Thanks in advance!

sylvainpolletvillard commented 1 year ago

Seems to work as intended on last version: https://codesandbox.io/p/sandbox/happy-dirac-3yh4d7?file=%2Findex.js&selection=%5B%7B%22endColumn%22%3A1%2C%22endLineNumber%22%3A23%2C%22startColumn%22%3A1%2C%22startLineNumber%22%3A23%7D%5D

Could you provide a reproducable example in a Codesandbox/Stackblitz/other ?

juanjoseruiz commented 1 year ago

Apologies for the confusion, I did a mistake when trying to isolate an example from the actual code.

The current model is a bit more sophisticated, and the problem seems to happen when nesting more than one Model. Here is the updated version:

https://codesandbox.io/p/sandbox/wizardly-night-hcv89r?file=%2Findex.js&selection=%5B%7B%22endColumn%22%3A11%2C%22endLineNumber%22%3A5%2C%22startColumn%22%3A11%2C%22startLineNumber%22%3A5%7D%5D

const { Model } = require("objectmodel");

const Address = new Model({
  street: String,
  number: String,
}).defaultTo({
  number: "unknown number",
});

const Person = new Model({
  name: String,
  address: Address,
}).defaultTo({
  name: "Unknown name",
});

const Register = new Model({
  person: Person,
});

const reg = new Register({ person: { address: { street: "unknown street" } } });
console.log(reg.person.address.number);
console.log(JSON.stringify(reg));

Getting an error right now:

TypeError: expecting person.address.number to be String, got undefined

If I make number property from Address optional, then it works as expected and outputs the default.

const Address = new Model({
  street: String,
  number: [String],
}).defaultTo({
  number: "unknown number",
});
sylvainpolletvillard commented 1 year ago

Thanks for the report ! Fixed and delivered in v4.4.2

juanjoseruiz commented 1 year ago

Thanks for the quick fix! I verified and I can confirm this fixes the problem with the provided example.

Unfortunately, this is still not working for the production code, which has different nested types (Objects / Arrays). I was playing with the code, and this seems to do the trick for me:

https://github.com/sylvainpolletvillard/ObjectModel/commit/a2106cea962c8366ada9bcac71f3165ffe3197b3

Could you please have a look to see if that makes sense? I'm just propagating that property to the rest of places where it may be needed, but probably I am missing something else.

Thanks! Juanjo.

sylvainpolletvillard commented 1 year ago

Possibly, what is the problem on your production code ? I need to add unit tests to cover the issue

juanjoseruiz commented 1 year ago

The problem is the same, the defaults are not applied / validation errors happens with several Model / ArrayModel / BasicModel nested.

I've slightly modified the example using the latest version so that you can check it (just substitute a Model by ArrayModel)

https://codesandbox.io/p/sandbox/wizardly-night-hcv89r?file=%2Findex.js&selection=%5B%7B%22endColumn%22%3A1%2C%22endLineNumber%22%3A9%2C%22startColumn%22%3A1%2C%22startLineNumber%22%3A9%7D%5D

Please, let me know if you need any further examples, but I think this one should cover the missing parts from the previous one.

Thanks and best regards, Juanjo.

sylvainpolletvillard commented 1 year ago

Thanks for the provided example, I'll add it to the unit tests

juanjoseruiz commented 1 year ago

Thanks for your help!

sylvainpolletvillard commented 1 year ago

After investigation, your second problem comes from a misunderstanding of ArrayModel#defaultTo ; defaultTo defines the default content of the array, not the defaults of the array item model.

Here is how to fix it: https://codesandbox.io/p/sandbox/quiet-http-8vszie?file=%2Findex.js&selection=%5B%7B%22endColumn%22%3A1%2C%22endLineNumber%22%3A28%2C%22startColumn%22%3A1%2C%22startLineNumber%22%3A1%7D%5D

const Address = new Model({
  street: String,
  number: String,
}).defaultTo({
  number: "unknown number",
});

const AddressList = new ArrayModel(Address);

const Person = new Model({
  name: String,
  address: AddressList,
}).defaultTo({
  name: "Unknown name",
});
sylvainpolletvillard commented 1 year ago

To prevent this kind of mistake, I added type-checking validation on the default value provided.

Now in v4.4.3, you would have this error message instead when passing a wrong value to defaultTo :

expecting Array, got Object {
    number: "unknown number" 
}
juanjoseruiz commented 1 year ago

Yes, the example was not fully accurate. After your changes in 4.4.3 now my code is working as previously.

Thanks!