Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

defaults() sets/buries values on __proto__ #75

Closed
gotjoshua opened this issue Jun 18, 2018 · 11 comments
Closed

defaults() sets/buries values on __proto__ #75

gotjoshua opened this issue Jun 18, 2018 · 11 comments

Comments

@gotjoshua
Copy link
Contributor

gotjoshua commented Jun 18, 2018

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
Copy link
Contributor Author

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
Copy link
Owner

Hi there,

Putting defaults in the prototype is the intended behaviour. I just explained this today in this other issue : #74 (comment)

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
Copy link
Contributor Author

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
Copy link
Owner

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

  • if you delete an instance property, it automatically gets back to the default value, inherited from the prototype (see the code example i used in issue 74), so the object stays valid.
  • you can change the default value of a model and it will automatically apply on all model instances, constructed in the past or in the future.
  • from a reference to a model instance, you can get the default value of a property even if has been overridden for the current instance

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

@gotjoshua
Copy link
Contributor Author

rock on.

great answers!

@sylvainpolletvillard
Copy link
Owner

sylvainpolletvillard commented Jun 18, 2018

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
Copy link
Owner

sylvainpolletvillard commented Jun 18, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@gotjoshua
Copy link
Contributor Author

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
Copy link
Owner

sylvainpolletvillard commented Jun 19, 2018

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: #76

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants