sylvainpolletvillard / ObjectModel

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

defaults() sets/buries values on __proto__ #75

Closed gotjoshua closed 6 years ago

gotjoshua commented 6 years ago

When I create a class that extends ObjectModel, with a required property (with a default set), and then create an instance, the default value is not added to the target, but rather buried down in the __proto__ (thus does not show up in the Chrome inspector Custom Formatter):

export class DailyFeeOM extends ObjectModel(
    {
        id: [Number],
        name: [String],
        isDefault: Boolean
    })
    .defaults({
        isDefault: true
    })
    {
    constructor(initObject) {
        super(initObject)
    }
}

let exampleOM = new DailyFeeOM(
    {
        name: 'newFeeNine',
        price: 9,
    }
)

console.log(exampleOM);

shows:

DailyFeeOM
{
name: "newFeeNine"
price: 9
}

i expect:

DailyFeeOM
{
name: "newFeeNine"
price: 9
isDefault: true
}
gotjoshua commented 6 years ago

of course the isDefault property is correctly set by the defaults() and accessible on DailyFeeOM.isDefault

but it is not easily inspectable, and I expect ObjectModel to set the default properties explicitly on the "instance".

Is the current behavior by design? desirable for some reason?

sylvainpolletvillard commented 6 years ago

Hi there,

Putting defaults in the prototype is the intended behaviour. I just explained this today in this other issue : https://github.com/sylvainpolletvillard/ObjectModel/issues/74#issuecomment-397988635

I explained it this way in the library docs (if it's not clear, I'll reword this section):

To specify default values for some properties of your object models, put them in the model prototype. You can also use the defaults method as a shorthand for setting all the default values at once.

Now you're right that I could show them in the custom formatter. I didn't do it in the first place because there are lots of other stuff in the prototype that seem useless to display, like constructor or toString methods. Maybe I can add a [Prototype] subsection, that could be an idea.

The same question applies on undeclared properties, i.e. existing properties that are not part of the model definition. Should we show them or not ?

gotjoshua commented 6 years ago

Hi again, well you mentioned it in #74 but i don't get the reason yet. (I do assume it makes sense because i trust you, i just don't get it yet)

I think it would be great if undeclared props on unfrozen instances would be inspectable, and also if the defaults [Prototype] props could be shown (and maybe even flagged in the inspector as defaults)

thanks again for your engagement!

sylvainpolletvillard commented 6 years ago

There are several reasons why the prototype is a great place to put default values:

I'll see what I can do to improve the current formatter and have a [Prototype] and [Undeclared] section

gotjoshua commented 6 years ago

rock on.

great answers!

sylvainpolletvillard commented 6 years ago

Current progress: image

I mimic the __proto__ property that is shown natively on Chrome for other objects.

Undeclared props are another problem, since they are not exposed by the proxy's ownKeys trap (another debatable choice). So I need to add a backdoor to get them

sylvainpolletvillard commented 6 years ago

Okay, I added undeclared props this way:

image

Thoughts ? Maybe the (undeclared) label is too much and I could just use a light gray color ?

gotjoshua commented 6 years ago

In general, i like it a lot. I also agree that (undeclared) is a bit bulky. Perhaps a smaller grey (+) could indicate that it is a "supplementary" prop that has been added to the instance.

Actually the expandable __proto__ business is also bulky, but it is accurate technically, so i like it.

gotjoshua commented 6 years ago

another option could be: (undec) for undeclared and (def) for __proto__ (instead of the expandable)

gotjoshua commented 6 years ago

Ah, just remembered (while you're into the custom formatting):

If I have a SetModel inside of a Model prop, it is not expandable in the custom formatter... would be nice.

(should i rather open a separate issue for this one?)

sylvainpolletvillard commented 6 years ago

prototype has to be expandable, first because it is another object and it would be incorrect to put these props on the same level, and secondly because some models have too much bloat in it.

Let me open another issue to discuss on the formatter: https://github.com/sylvainpolletvillard/ObjectModel/issues/76