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

Implements _super() for computed properties #1393

Merged
merged 1 commit into from Nov 1, 2012

Conversation

Projects
None yet
5 participants
@tomdale
Member

tomdale commented Sep 20, 2012

See #607

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Sep 21, 2012

Member

@tomdale I imagine you have some concerns about this solution since you didn't merge immediately. Can you share what those are?

Member

wagenet commented Sep 21, 2012

@tomdale I imagine you have some concerns about this solution since you didn't merge immediately. Can you share what those are?

@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale Sep 21, 2012

Member

Primarily I just wanted @kselden to review it to ensure I am not causing any major perf regressions.

Additionally, we should discuss if this is even a good feature to have at all. For example, it might surprise developers that dependent keys are not inherited, even if they call this._super(). If we did implement dependent key inheritance, that opens up a whole other can of API worms, because now we would need to provide a mechanism for when you do not want DKs inherited.

Member

tomdale commented Sep 21, 2012

Primarily I just wanted @kselden to review it to ensure I am not causing any major perf regressions.

Additionally, we should discuss if this is even a good feature to have at all. For example, it might surprise developers that dependent keys are not inherited, even if they call this._super(). If we did implement dependent key inheritance, that opens up a whole other can of API worms, because now we would need to provide a mechanism for when you do not want DKs inherited.

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 21, 2012

Member

we do Ember.meta(obj) in applyMixin() and mixinsMeta() and now here. Maybe it can just be done once in applyMixin, then make mixinsMeta be setupMixinsMeta(m, obj) and pass in the meta to mergeMixins?

Member

krisselden commented on packages/ember-metal/lib/mixin.js in cae0447 Sep 21, 2012

we do Ember.meta(obj) in applyMixin() and mixinsMeta() and now here. Maybe it can just be done once in applyMixin, then make mixinsMeta be setupMixinsMeta(m, obj) and pass in the meta to mergeMixins?

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 21, 2012

Member

Looks good, we can do another round of perf measuring soon.

I think it is important that the dependency keys be concatenated. I think in general, this goes with moving setters to being their own function, like setFoo, and create calling setters instead of overriding. I think CP logic is becoming more of a first class API citizen, and people should be afraid of overriding without calling super because they are likely to break how a class works.

Member

krisselden commented Sep 21, 2012

Looks good, we can do another round of perf measuring soon.

I think it is important that the dependency keys be concatenated. I think in general, this goes with moving setters to being their own function, like setFoo, and create calling setters instead of overriding. I think CP logic is becoming more of a first class API citizen, and people should be afraid of overriding without calling super because they are likely to break how a class works.

@ebryn

This comment has been minimized.

Show comment
Hide comment
@ebryn

ebryn Sep 21, 2012

Member

I'm fine with this as it is in the short term, but I'd like us to move ahead with the changes we've been discussing: changing the create behavior to call CP setters and splitting out CP setters into separate functions.

@kselden and I both think we could get rid of the CP descriptors and just mark CP functions like we do with observers.

Member

ebryn commented Sep 21, 2012

I'm fine with this as it is in the short term, but I'd like us to move ahead with the changes we've been discussing: changing the create behavior to call CP setters and splitting out CP setters into separate functions.

@kselden and I both think we could get rid of the CP descriptors and just mark CP functions like we do with observers.

@ebryn

This comment has been minimized.

Show comment
Hide comment
@ebryn

ebryn Sep 21, 2012

Member

Also, I do believe that the default behavior should be to concatenate the DKs when super is being called. From my tests, we can use Function#toString to detect this in IE6+.

Member

ebryn commented Sep 21, 2012

Also, I do believe that the default behavior should be to concatenate the DKs when super is being called. From my tests, we can use Function#toString to detect this in IE6+.

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 21, 2012

Member

If we need proxies to support unknownProperty then we don't need the desc and m.values because we are already hooking get/set through the proxy. also, we probably should add a respondsTo to support the in operator with unknownProperty.

Member

krisselden commented Sep 21, 2012

If we need proxies to support unknownProperty then we don't need the desc and m.values because we are already hooking get/set through the proxy. also, we probably should add a respondsTo to support the in operator with unknownProperty.

function removeKeys(keyName) {
delete descs[keyName];
delete values[keyName];
}
function cloneDescriptor(desc) {
var newDesc = new Ember.ComputedProperty();

This comment has been minimized.

@wycats

wycats Sep 21, 2012

Member

Couldn't we just Ember.create the original descriptor here? @kselden

@wycats

wycats Sep 21, 2012

Member

Couldn't we just Ember.create the original descriptor here? @kselden

This comment has been minimized.

@krisselden

krisselden Sep 21, 2012

Member

absolutely, good catch, you don't even need the cloneDescriptor call at all

@krisselden

krisselden Sep 21, 2012

Member

absolutely, good catch, you don't even need the cloneDescriptor call at all

// same parent, we need to clone the computed
// property so that other mixins do not receive
// the wrapped version.
value = cloneDescriptor(value);

This comment has been minimized.

@krisselden

krisselden Sep 21, 2012

Member
value  = o_create(value);
value.func = Ember.wrap(value.func, ovalue.func);
@krisselden

krisselden Sep 21, 2012

Member
value  = o_create(value);
value.func = Ember.wrap(value.func, ovalue.func);
@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Oct 15, 2012

Member

@kselden you mentioned that you wanted to concatenate the dependent keys of super'ed computed properties. Should I wait on that or just accept this?

Member

wycats commented Oct 15, 2012

@kselden you mentioned that you wanted to concatenate the dependent keys of super'ed computed properties. Should I wait on that or just accept this?

tomdale added a commit that referenced this pull request Nov 1, 2012

Merge pull request #1393 from emberjs/cp-super
Implements _super() for computed properties

@tomdale tomdale merged commit 0ac8748 into master Nov 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment