jneen / pjs

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

Calling .constructor on an instance with arguments seems to break. #9

Closed jwmerrill closed 11 years ago

jwmerrill commented 11 years ago

I'm trying to write a factory method that constructs instances of a subtype:

P(function (proto) {
  proto.factory = function(a1, a2) {return this.constructor(a1, a2)};
});

but using this.constructor with arguments doesn't succeed in passing the arguments to init.

Here's a jsfiddle with running code: http://jsfiddle.net/gKrK3/

jneen commented 11 years ago

Haha, I think I know what's going on. You're hitting the instanceof check because you're using the method calling convention, which sets this to the instance. As a workaround, you can stash it in a variable:

var constructor = this.constructor;
return constructor(a1, a2);

Alternatively, what if you were to put the factory on the class itself? The this context of the definition function is the class itself:

var MyClass = P(function(proto) {
  this.factory = function(a1, a2) { return this(a1, a2); };
});

Looks kinda weird calling this() as a function, but it totally works.

jneen commented 11 years ago

Another alternative is to use the new keyword with the varargs syntax:

return new (this.constructor)([a1, a2])

which also isn't great. Unfortunately I don't think there's anything that can be done about this, since foo.bar(...) always calls foo.bar with a context of foo. Unless you have any ideas?

jwmerrill commented 11 years ago

Your first suggestion works best in my context.

This seems like a pretty annoying sharp edge, though, since I'd guess 90% of the uses of proto.constructor would be to call it right away.

What about passing an extra flag when C is calling itself?

function C(args, fromself) {
  var self = this;
  if (!fromself || !(self instanceof C)) return new C(arguments, true);
  if (args && isFunction(self.init)) self.init.apply(self, args);
}

Haven't tested this--I can look at it again tomorrow.

jwmerrill commented 11 years ago

Looks like my suggestion wouldn't work in the case of calling

this.constructor(a1, false)
jneen commented 11 years ago

Yeah, that argument signature is misleading - hence the page-long comment. It's actually varargs when called without new.

jneen commented 11 years ago

Oh, forgot to mention - this is fixed in 3.x. We even added a test!

jneen commented 10 years ago

In v5.x, you'll need to add a new keyword to the call to this.constructor(...), since otherwise it will mutate the caller.