Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fixes #1966: external constructors should produce their return value #1970

Merged
merged 1 commit into from

5 participants

@michaelficarra
Collaborator

I'm not 100% on this, but it seems better than the alternative, which would be to treat the return value of already-defined functions differently when pointed to as an external constructor.

edit: I'm pretty comfortable with it now. See my comment below.

@TrevorBurnham
Collaborator

Yikes, new A not instanceof A? Why allow external constructors at all?

@michaelficarra
Collaborator
class A then constructor: -> return {}
ctor = -> {}
class B then constructor: ctor
(new A) not instanceof A # yep
(new B) not instanceof B # same result

That's not undesirable behaviour. It's consistent. The class syntax is just a sugar for adding prototype properties in an IIFE. Which constructor we're talking about is specified with the special constructor key. There's three options:

  • omit it, the compiler will make an empty one for us
  • give it a function literal, compiler will ignore implicit returns
  • give it a reference/expression
    • currently, we save the referenced/generated function, but always ignore its return value
    • this proposed change will cause the constructor to behave more like we're actually using that function as the constructor
@jnicklas

If this patch is accepted both returning the new instance, and returning something else (which is useful) would be possible, since we can still return this from the dynamically assigned constructor function. So I would say that this is more flexible than the current behavior. However, it is an API breaking change, which is somewhat unfortunate.

@rymohr

Thanks Michael. Your solution is much more elegant. Wasn't aware of the makeReturn() method.

I actually saw this pull request before but found the test case hard to decipher so I thought I'd take a stab at it, if only to clarify why this behavior is desirable.

With this modification, adding an identity map to Backbone is just a few lines of code when all other solutions I've seen are forced to duplicate Backbone.Model instead of extend it.

@michaelficarra
Collaborator

@islandr: glad it will help you.

@jnicklas

Identity map is the exakt use case I've been toying around with this as well, thank you for merging, will have to play with it and see how it works now!

@michaelficarra
Collaborator

@jnicklas: It hasn't been merged yet. I'm waiting for approval from @jashkenas and hopefully @trevorburnham will come around.

@jashkenas
Owner

Yep -- thanks for the pull req.

@jashkenas jashkenas merged commit 4a0e813 into from
@rymohr

Awesome. Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
1  lib/coffee-script/nodes.js
@@ -1255,6 +1255,7 @@
if (this.externalCtor) {
this.ctor.body.push(new Literal("" + this.externalCtor + ".apply(this, arguments)"));
}
+ this.ctor.body.makeReturn();
this.body.expressions.unshift(this.ctor);
}
this.ctor.ctor = this.ctor.name = name;
View
1  src/nodes.coffee
@@ -923,6 +923,7 @@ exports.Class = class Class extends Base
@ctor = new Code
@ctor.body.push new Literal "#{name}.__super__.constructor.apply(this, arguments)" if @parent
@ctor.body.push new Literal "#{@externalCtor}.apply(this, arguments)" if @externalCtor
+ @ctor.body.makeReturn()
@body.expressions.unshift @ctor
@ctor.ctor = @ctor.name = name
@ctor.klass = null
View
6 test/classes.coffee
@@ -490,6 +490,7 @@ test "#1182: execution order needs to be considered as well", ->
test "#1182: external constructors with bound functions", ->
fn = ->
{one: 1}
+ this
class B
class A
constructor: fn
@@ -604,3 +605,8 @@ test "#494: Named classes", ->
class A.B["C"]
ok A.B.C.name isnt 'C'
+
+test "#1966: external constructors should produce their return value", ->
+ ctor = -> {}
+ class A then constructor: ctor
+ ok (new A) not instanceof A
Something went wrong with that request. Please try again.