-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix registerPrimitive property merging (fixes #1324) #1332
Conversation
|
||
self.setAttribute(componentName, propName, componentData[propName]); | ||
}); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we should return
only if self.setAttribute(…)
actually got called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only returning here to mimic an if/else
. Not a big fan of that style since it's less clear on the intent, but dmarcos likes that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I guess I understand
applyDefaultComponents: { | ||
value: function () { | ||
var self = this; | ||
var defaultData = utils.extendDeep(this.defaultAttributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary to create a local copy of the object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. if you modify this object, it will affect all other entities created from this prototype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where is the defaultData object modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think since I removed keeping track of component data, this is no longer needed. Good catch.
Description: Mapped attributes were overwriting defined components.
Changes proposed:
setAttribute(component, propName, propValue)
syntax.