Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement new Generator.extend inheritance method #446

Merged
merged 1 commit into from Dec 27, 2013

Conversation

SBoudrias
Copy link
Member

I'll add some missing test, but this is my WIP.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0d43026 on SBoudrias:inheritance into c1210d0 on yeoman:master.

};

/**
* Extend this Class to create a new one inherithing this one.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just use or wrap util.inherits?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the end user, the advantage is mostly the syntax sugar.

Internally, util.inherits simply call object.create and assign a _super method to the constructor. It is a very light wrapper, and I prefer defining our own __super__ reference as I found this way more useful. Also, here we can declare both instances and static method which is an advantage over relying simply on the node.js inheritance helper.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the bikeshedding. I just don't want us to bloat it with unnecessary code if not needed. It would still be possible to add static properties with util.inherits, right? I guess I don't see the convenience.

Isn't there already a module on npm that does this? It feels very generic. If not, could you do it as a separate module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

util.inherits really just a small wrapper over assigning a prototype inheritance via object.create https://github.com/joyent/node/blob/master/lib/util.js#L558-L581 - so there's no static built-in.

There's one similar node module out there that I know of (though it does way more) -> https://github.com/dfilatov/inherit

But this is really core to the way yeoman generators are extended, I really feel this should belong close to them (with direct unit tests) for more stability.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is really core to the way yeoman generators are extended, I really feel this should belong close to them (with direct unit tests) for more stability.

Yes, but this is Node, and things should be modular if possible. The generator core is way too big for what it does and we should try to separate out more of it.

When you need to add static methods, can't you just use normal _.extend() for that?

@SBoudrias
Copy link
Member Author

There, I extracted this functionality in its own node module.

I removed some unit tests, but I kept the main feature's ones as it is quite important to ensure inheritance works well when we send out releases.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling cc21acf on SBoudrias:inheritance into c1210d0 on yeoman:master.

sindresorhus added a commit that referenced this pull request Dec 27, 2013
Implement new Generator.extend inheritance method
@sindresorhus sindresorhus merged commit c549c67 into yeoman:master Dec 27, 2013
@sindresorhus
Copy link
Member

Awesome! Thanks @SBoudrias :)

tumblr_mndj28lzs21qdlh1io1_250

@SBoudrias SBoudrias deleted the inheritance branch December 27, 2013 00:54
@SBoudrias
Copy link
Member Author

hahaha no problem

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

Successfully merging this pull request may close these issues.

None yet

3 participants