-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
positionalParams causing extra change notifications on initial render #11686
Comments
@ef4 I've assigned you, to at least provide feedback, since you are likely most familiar with this. |
I concur that the positionalArgs array itself is intended to be a static property of the class, and it should be fair game to access it before instantiation of a component instance. |
I will document + assert accordingly, and then take advantage of this concept to being addressing issues the compat code has introduced. |
Ok, i think we will treat positionArgs as if it was static, and write some assertions to ensure users don't use it any other way. The final resting place will likely be static, but we can support bothwithout cost today. This will enable us to begin fixing #11502 (comment) |
Brain dump after discussing with @stefanpenner in chat: Go forward would be: var Thing = Ember.Component.extend({
// stuff here
});
Thing.reopenClass({
positionalParams: ['stuff', 'here']
});
export default Thing; Deprecated but functional: export default Ember.Component.extend({
positionalParams: ['stuff', 'here']
}); |
So today, we must create the component, extract the position args and then place them on
attrs
. This means we currently always dirtythis.attrs
even on initial render. At the very best this results in wasteful changeEvents, at the worse may cause a hefty work amplification.I feel strongly, we need to work hard to ensure
attrs
on initial render is in its fully formed initial state.Some ideas:
create
.component.init
note: it is true that we also invalidate the keys on
this.attrs
on initial render, we also need to solve that. But both this and that are tightly related.The text was updated successfully, but these errors were encountered: