Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

compose and prototype.constructor #13

Closed
kitsonk opened this issue Nov 13, 2015 · 3 comments
Closed

compose and prototype.constructor #13

kitsonk opened this issue Nov 13, 2015 · 3 comments
Assignees
Milestone

Comments

@kitsonk
Copy link
Member

kitsonk commented Nov 13, 2015

In conversations with other developers, the question was raised "what happens when you mixin in a method named constructor into a compose class"?

Should we guard against methods named constructor being mixed in and throw?

@kitsonk kitsonk added this to the alpha.2 milestone Mar 11, 2016
@kitsonk kitsonk modified the milestones: 2016.04, alpha.2 Apr 8, 2016
@mwistrand
Copy link

Resetting prototype.constructor is standard fare in classical JS inheritance schemes, so throwing an error for any base function passed to compose that happens to do so (e.g., third-party modules installed via Bower) could prove problematic. Since constructor functions of other bases are ignored, it would be nice if these constructors were treated the same. In order to prevent these constructors from overriding factory.prototype.constructor, they could be deliberately excluded from the internal copyProperties method, or factory.prototype.constructor = factory could placed after the call to copyProperties within compose, instead of within cloneFactory.

@maier49
Copy link
Contributor

maier49 commented Apr 13, 2016

As far as I can tell, a base having a method called constructor isn't really a problem. If there's a method called constructor on the base, the resulting factory will just create instances that also have a constructor method. I'm not sure why somebody would have a method called constructor but it doesn't seem like it would cause any more problems for compose than it would in the first place.

In the case that the constructor is actually on the base.prototype it seems like @mwistrand 's suggestion would be sufficient to handle this. This would just require adding one or two lines to create and extend, as mixin passes the object through create if it's not a ComposeFactory anyway.

@kitsonk kitsonk modified the milestones: 2016.05, 2016.04 Apr 26, 2016
@kitsonk kitsonk modified the milestones: 2016.06, 2016.05 Jun 7, 2016
@kitsonk kitsonk modified the milestones: 2016.06, 2016.07 Jul 4, 2016
@kitsonk kitsonk modified the milestones: 2016.07, 2016.08 Aug 1, 2016
@kitsonk kitsonk modified the milestones: 2016.10, 2016.08 Sep 29, 2016
@kitsonk kitsonk self-assigned this Sep 29, 2016
@kitsonk
Copy link
Member Author

kitsonk commented Sep 29, 2016

Agreed that we should exclude constructor from copyProperties.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants