Change to sc_super()

[Tom Dale]

[Tom Dale]

Hey guys,

Yehuda and I have been working on some changes to the way reopen() works. Specifically, we've been investigating some weirdness with the use of sc_super() and the new, reopened versions of the framework's classes. (In this instance, we were investigating the case where Cmd-R and other browser shortcuts were getting consumed.)

We noticed that many classes call sc_super() when there is no super version of that method. For example, imagine this case:

var MyClass = SC.Object.extend({});

var MySubClass = MyClass.extend({
print: function() {
console.log("foo");
sc_super();
}
});

var myObject = MySubClass.create().print();

Currently, the call to sc_super() will result in a no-op. However, we've recently discovered several bugs that were caused by this behavior (where, apparently, people were expecting a mixin's method to be called instead of a superclass).

We've pushed some code to master 1.5 that now throws an exception if SproutCore cannot find a superclass implementation of a method. If you encounter errors after rebasing, you should be able to easily solve this problem by removing the call to sc_super() (which never had an effect anyhow).

Please let us know if you have any problems with the new code.

Thanks,
Tom

[Martin Häcker]

Hi Tom / Yehuda,

Am 11.02.11 01:20, schrieb Tom Dale:

> Please let us know if you have any problems with the new code.

What is the policy you want to set with regards to this new super code
and mixins?

Will different mixins be able to override each others methods and use
sc_super() to chain them together?

That is something I recognized in some of our code (and likely in
sproutcore) and it can be a very hard to debug issue if its just handled
by replacing the methods from a different mixin.

Are you also on the bug I reported earlier where using a mixin in
different classes would always use the super method of the last object
the mixin was mixed in?

Regards,
Martin

[Tom Dale]

Hi Martin,

Currently, mixins do not introduce any kind of superclass relationship. In other words, mixins do not have access to the methods they override, whether via sc_super() or some other means.

There are two schools of thought on this.

My personal feeling is that mixins are mixins and adding them as what amounts to an ad-hoc parent class is confusing and muddies the waters conceptually of what a mixin is.

There are others who want to inherit mixin behavior from Ruby, where sc_super() calls the mixin's implementation first (if it exists), and then you can also call sc_super() from the mixin, which will either call the next mixin's implementation (if it exists), or the parent class' implementation if there are no further mixins which contain the method.

I'm not married to my stance and am willing to be convinced either way.

Best,
Tom

[Martin Häcker]

Hi Tom,

Am 11.02.11 16:23, schrieb Tom Dale:

> Currently, mixins do not introduce any kind of superclass
> relationship. In other words, mixins do not have access to the
> methods they override, whether via sc_super() or some other means.

Well, if with currently you mean sproutcore 1.4.5, then they do - I've
tried it.

> There are two schools of thought on this.
>
> My personal feeling is that mixins are mixins and adding them as what
> amounts to an ad-hoc parent class is confusing and muddies the waters
> conceptually of what a mixin is.
>
> There are others who want to inherit mixin behavior from Ruby, where
> sc_super() calls the mixin's implementation first (if it exists), and
> then you can also call sc_super() from the mixin, which will either
> call the next mixin's implementation (if it exists), or the parent
> class' implementation if there are no further mixins which contain
> the method.
>
> I'm not married to my stance and am willing to be convinced either
> way.

I'm also not having a stance on this, as long as its clearly documented
and has a clearly and easy to debug way of finding out what happens.
I.e. if mixins are mixins and just replace existing methods (either from
the class or another mixin) then there should be an error message thrown
/ logged so you can easily isolate that problem. This of course makes it
harder to patch existing methods as around advices.

In the other case it needs to be clearly indicated in the output if you
look at the method name from which mixin it is coming and how many more
mixin 'super' methods are there on that class from other mixins.

What I want is just a well defined easy to understand and easy to debug
working of the system as the tools for js development are far less
advanced than for almost any other language (so I would favour simplicity).

Regards,
Martin

[Alex Iskander]

Le Feb 11, 2011 à 10:47 AM, Martin Häcker a écrit :

> Am 11.02.11 16:23, schrieb Tom Dale:
>> Currently, mixins do not introduce any kind of superclass
>> relationship. In other words, mixins do not have access to the
>> methods they override, whether via sc_super() or some other means.
>
> Well, if with currently you mean sproutcore 1.4.5, then they do - I've
> tried it.

They don't. If you use it, Bad Things Happen. This is because when they are mixed in, the 'base' property of each function on the mixin is set to the parent class's method. That's fine. But if you then mix it in to another parent class, it has to either a) set the 'base' property to the new parent's method, or b) leave it as-is.

This leads to really weird crashes, but unfortunately, fails silently.

In short: never use sc_super from a mixin.

Alex

[Tom Dale]

After thinking about this some more (and discussing it with Yehuda), I think the solution is to modify mixins so that they support the same enhance() syntax that reopen() does. I think the decorator pattern is more appropriate here than some kind of inheritance pattern.

For example:

var exclamationMixin = {
print: function(original, msg) {
original(msg+"!");
}
};

var MyClass = SC.Object.extend({
print: function(msg) {
console.log(msg);
}
});

var myObj = MyClass.create(exclamationMixin);

myObj.send("Hello world"); // should print "Hello world!"

[Alex Iskander]

Agreed, except that I think you should still have to explicitly state `.enhance()` (your example doesn't; maybe you intended it, though).

Alex

[Tom Dale]

Sorry; you're right, I meant to include .enhance().