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

Merged
merged 1 commit into from Dec 27, 2011

Conversation

Projects
None yet
5 participants
Collaborator

michaelficarra commented Dec 24, 2011

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.

Collaborator

TrevorBurnham commented Dec 24, 2011

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

Collaborator

michaelficarra commented Dec 26, 2011

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

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 commented Dec 27, 2011

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.

Collaborator

michaelficarra commented Dec 27, 2011

@islandr: glad it will help you.

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!

Collaborator

michaelficarra commented Dec 27, 2011

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

Owner

jashkenas commented Dec 27, 2011

Yep -- thanks for the pull req.

jashkenas added a commit that referenced this pull request Dec 27, 2011

Merge pull request #1970 from michaelficarra/issue1966
fixes #1966: external constructors should produce their return value

@jashkenas jashkenas merged commit 4a0e813 into jashkenas:master Dec 27, 2011

rymohr commented Dec 27, 2011

Awesome. Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment