ded / klass

a utility for creating expressive classes in JavaScript
751 stars 78 forks source link

Discuss: Avoid wrapping subclass methods #4

Closed ghost closed 13 years ago

ghost commented 13 years ago

Currently, Klass.js wraps subclass methods by using function decompilation to detect a call to this.supr() and. This not only incurs a performance overhead, but exposes a leaky abstraction if the subclass method throws an exception. Consider the following example:

var Person = klass(function(name) {
  this.name = name;
}).methods({
  'walk': function() {
    console.log('Walking...');
  }
});

var SuperHuman = Person.extend(function(name) {
  // ...
}).methods({
  'supr': function() {
    console.log('Exposed method.');
  },
  'walk': function() {
    this.supr();
    console.log('Flying...');
  }
});

var kit = new SuperHuman('Kit');
kit.walk();

// => Walking...
// => Flying...

kit.supr();
// => Exposed method.

SuperHuman.implement({
  'walk': function() {
    this.supr();
    throw new Error('Exception.');
  }
});

kit.walk();
// => Error: Exception.

kit.supr;
// => Oops; `kit.supr` now references `Person#walk`.

kit.supr();
// => Walking...

In other words, if the subclassed method throws an exception, the original value of the supr method is not restored. A possible solution would be to wrap the execution of the method in a try...catch block; however, this will result in additional detrimental effects to performance. Alternatively, we could remove wrapping entirely, and simply provide a reference to the original superclass method as a property on the subclassed method. Incidentally, this technique was previously outlined in an article by T.J. Crowder. I think it's more robust, both in terms of performance and mitigating leaky abstractions, than the wrapping technique that Klass.js currently uses. We've also adopted T.J.'s technique in FuseJS, in place of Prototype's more convoluted inheritance scheme.

I would be happy to submit a pull request for this if you'd like, or even provide additional examples, but I'm interested in knowing if you think this is a good idea or not first.

ded commented 13 years ago

@kitgoncharov +1 on this. I'd love to see a working pull-request. Your examples provide enough insight — and as long as the interface remains the same, we could help in testing your integration.

fat commented 13 years ago

Yeah that would be much appreciated, thanks for pointing that out!

stephenmelrose commented 13 years ago

+1 on this. For reference, I believe this is the approach Object.subClass() uses?

ghost commented 13 years ago

Closing this issue as per the discussion in pull request #6.