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

Already on GitHub? Sign in to your account

Remove bower dependency #201

Merged
merged 1 commit into from Apr 7, 2013

Conversation

Projects
None yet
3 participants
Member

passy commented Apr 7, 2013

After some hours of profiling I found that the bower dependency accounted for a substantial part of the start-up time. After removing bower a stripped-down version of generator-webapp ran in 0.3s compared to 1.7s with bower. For poor Sindre, who's still running on an HDD, the startup time went from 24s to 1.25s.

The reason for this is that bower is recursively required by all sub-generators and bower itself has quite a lot of dependencies, causing more than 22 000 synchronous stat syscalls when looking up dependent modules in my test case with two sub-generators.

This patch removes the bower dependency and uses childprocess.spawn instead to provide the same functionality, as suggested by @mklabs.

EDIT:

Scientific comparison

2013-04-08 00 14 56

test/actions.js
@@ -4,6 +4,8 @@ var path = require('path');
var util = require('util');
var events = require('events');
var assert = require('assert');
+var proxyquire = require('proxyquire');
+var sinon = require('sinon');
@passy

passy Apr 7, 2013

Member

Oh yeah, previous experiment.

-// TODO(mklabs): bower has quite a few dependencies, just like npm, consider
-// spawning the `bower` command instead, with some gracefull fallback to
-// local `./node_modules/.bin` folder and error handling
@sindresorhus

sindresorhus Apr 7, 2013

Owner

gracefull fallback to local ./node_modules/.bin folder and error handling

Should we?

@passy

passy Apr 7, 2013

Member

I chose not to, because bower is required to run Yeoman anyway. I don't think there's much use from this.

Member

kevva commented Apr 7, 2013

Can't say no to science!

lib/actions/bower.js
.on('error', cb)
- .on('end', cb)
- .on('end', this.emit.bind(this, 'install:end', paths));
+ .on('exit', cb)
@sindresorhus

sindresorhus Apr 7, 2013

Owner

If it exits with not found code, 127 i think, show the user an error message about installing Bower

+ });
+
+ return args;
+}
@passy

passy Apr 7, 2013

Member

This is from grunt-lib-contrib. I'd like to get some comments on this. Is it okay to copy it for this purpose?

@sindresorhus

sindresorhus Apr 7, 2013

Owner

Yes. I wrote it. Working on extracting it to a separate lib. This will do for now.

Owner

sindresorhus commented Apr 7, 2013

BEST PR EVER!

Member

passy commented Apr 7, 2013

Added an error message if bower isn't found.

sindresorhus added a commit that referenced this pull request Apr 7, 2013

@sindresorhus sindresorhus merged commit a9b7da5 into yeoman:master Apr 7, 2013

1 check passed

default The Travis build passed
Details
Owner

sindresorhus commented Apr 7, 2013

Incredible! Nice work @passy

Glad we got this sorted out.

Member

passy commented Apr 7, 2013

\o/ Thanks for the help! I hope we can make it even faster.

For the future, we should be really careful when adding further dependencies to this module.

Member

kevva commented Apr 7, 2013

👍 made a huge difference in terms of performance. Some real good work.

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