Skip to content
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

Allow calling this._super in subclasses of CoreObject. #1721

Merged
merged 1 commit into from
Aug 24, 2014

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Aug 21, 2014

  • Add a few tests for CoreObject.
  • Allow calling methods on your parent class via this._super.someMethod().

Illustration:

var fooCalled, barCalled, instance;

var Klass1 = CoreObject.extend({
  foo: function() {
    fooCalled = true;
  }
});

var Klass2 = Klass1.extend({
  foo: function() {
    this._super.foo();
  },

  bar: function() {
    barCalled = true;
    this._super.foo();
  }
});

instance = new Klass2();
instance.bar();

assert(fooCalled, 'foo called');
assert(barCalled, 'bar called');

fooCalled = false;
instance.foo();

assert(fooCalled, 'foo called');

@dgeb
Copy link
Member

dgeb commented Aug 21, 2014

@rwjblue This is nice and lightweight and gets the job done. 👍

Let me know if you're interested in a solution that allows calls to this._super() from within methods to call the corresponding method on the superclass, without needing to call this.super.foo(). Obviously, my suggestion is more involved, but may be more familiar to Ember devs.

@rwjblue
Copy link
Member Author

rwjblue commented Aug 21, 2014

Updated to include a super wrapper (basically the same implementation as Ember), with a few tweaks.

I attempted to make this conceptually similar to ES6 classes super (see here).

@rwjblue rwjblue force-pushed the allow-calling-super branch 2 times, most recently from df7952c to f7f638b Compare August 21, 2014 18:42
@rwjblue rwjblue changed the title Allow calling super in subclasses of CoreObject. Allow calling this._super in subclasses of CoreObject. Aug 21, 2014
@bcardarella
Copy link
Contributor

@rwjblue failed

@rwjblue
Copy link
Member Author

rwjblue commented Aug 22, 2014

I'm not exactly sure what the deal here is. It is only failing under node 0.11...

@rwjblue rwjblue force-pushed the allow-calling-super branch 6 times, most recently from fb6b449 to 9db1817 Compare August 24, 2014 13:21
@rwjblue
Copy link
Member Author

rwjblue commented Aug 24, 2014

Tracked down the issue in node 0.11:

Apparently, setting name on a function is not allowed (see here). Fixed with a simple check to ensure we don't override native function properties or methods in the super wrapper.

@rwjblue rwjblue force-pushed the allow-calling-super branch 2 times, most recently from 92c5aa7 to e16d1de Compare August 24, 2014 21:11
@rwjblue
Copy link
Member Author

rwjblue commented Aug 24, 2014

Updated to ensure we do not wrap methods that do not call _super.

@stefanpenner
Copy link
Contributor

I really dislike ember's super, can we just do it manually like:

this._super$foo(..)

https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/map.js#L350
https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/map.js#L363

its ugly but "just works"

@rwjblue
Copy link
Member Author

rwjblue commented Aug 24, 2014

My initial implementation actually, just made _super the super class's prototype. This allowed for this._super.foo() (which is sorta close to the ES6 way).

Will revert to that implementation (which was really just a single line change IIRC).

* Add a few tests for `CoreObject`.
* Allow calling methods on your parent class via `this._super.someMethod()`.

Illustration:

```javascript
var fooCalled, barCalled, instance;

var Klass1 = CoreObject.extend({
  foo: function() {
    fooCalled = true;
  }
});

var Klass2 = Klass1.extend({
  foo: function() {
    this._super.foo();
  },

  bar: function() {
    barCalled = true;
    this._super.foo();
  }
});

instance = new Klass2();
instance.bar();

assert(fooCalled, 'foo called');
assert(barCalled, 'bar called');

fooCalled = false;
instance.foo();

assert(fooCalled, 'foo called');
```
@rwjblue
Copy link
Member Author

rwjblue commented Aug 24, 2014

Moved back to super simple _super. Will merge when green...

rwjblue added a commit that referenced this pull request Aug 24, 2014
Allow calling this._super in subclasses of CoreObject.
@rwjblue rwjblue merged commit 3b54648 into ember-cli:master Aug 24, 2014
@rwjblue rwjblue deleted the allow-calling-super branch August 24, 2014 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants