jneen / pjs

Classes for javascript that don't suck.
MIT License
195 stars 29 forks source link

Fix idiomatic subclasses of Pjs classes #backcompatBreaking #20

Closed laughinghan closed 10 years ago

laughinghan commented 10 years ago

Prior to this commit an idiomatic subclass of a Pjs class would fail to inherit the constructor, for example:

function Subclass(info, moreInfo) {
  // ...
  var arg1 = slightlyChange(info), arg2 = drasticallyChange(moreInfo);
  // ...
  Superclass.call(this, arg1, arg2);
}

would silently create and discard a new instance of Superclass and not mutate as this as intended.

Of course, if you're writing your idiomatic subclass of a Pjs class by hand, you can just do this.init(arg1, arg2) and you're golden. However, in CoffeeScript and even ECMAScript 6 (Harmony), super(arg1, arg2) compiles to Superclass.call(this, arg1, arg2), and if you don't define a constructor, there's an implicit Superclass.apply(this, arguments), which it's pretty lame to break.

The fix is a one-liner:

     function C() {
-      var self = new Bare;
+      var self = this instanceof C ? this : new Bare;
       if (isFunction(self.init)) self.init.apply(self, arguments);
       return self;
     }

The only back-compat break is jneen#9: this.constructor(arg1, arg2) will also mutate this, being fundamentally equivalent to C.call(this, arg1, arg2). But it occured to me that this isn't a problem for idiomatic classes because to instantiate them you do new this.constructor(arg1, arg2), which we can make work for Pjs classes too, and just say that in that ambiguous case you use new to "force" instantiation. Note that for varargs you just do this.constructor.apply(null, argsList) as ever. (Btw we totally forgot to suggest this.constructor.call(null) in jneen#9.)

I really think that given the tradeoff of being a well-behaved class system vs the edge case of a factory method, especially considering how easy it is to just use new, we should favor being well-behaved.

The minified size does increase a smidgen, but fret not, I saw some ways to reduce minified size that I'll open separate PRs for. (Avoiding the repeated this intro'd in this change does not in fact reduce minified size, I tried that.)

jneen commented 10 years ago

:+1:

Allowing coffeescript and ES6 classes to subclass pjs seems pretty important, and #9 is pretty edge-case anyways. And like you said, just add a new in that case and everything works fine.