-
-
Notifications
You must be signed in to change notification settings - Fork 299
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
Find all node_modules directories, not only nearest to root #442
Conversation
nodeModules.forEach(function (root) { | ||
if (!root) return; | ||
var found = glob.sync('generator-*', { cwd: root, stat: true }) | ||
.map(function( match ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong style guide 👅
This will require some unit test. Also to make sure the generators are registered in the good older. Nearet |
if (!root) return; | ||
var found = glob.sync('generator-*', { cwd: root, stat: true }) | ||
.map(function( match ) { | ||
.forEach(function (root) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for a line return before the forEach
block.
I'm not sure about the way you refactored this. There's now two What I meant earlier was:
|
Done. This line seems strange for me, because it is not clear, why concatenate in reverse order, but it does the needed behaviour - |
|
||
describe('when there\'s ancestor node_modules/ folder', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after function
keyword
I think we should also add a test to make sure local modules are prioritize over global modules. Mind taking care of this one as you're already on it? |
@@ -20,6 +20,7 @@ describe('Environment Resolver', function () { | |||
shell.exec('npm install', { silent: true }); | |||
shell.exec('npm install generator-jquery', { silent: true }); | |||
shell.exec('npm install -g generator-angular', { silent: true }); | |||
shell.exec('npm install -g generator-dummy', { silent: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can put both on the same line so we only spawn a single process: npm install -g generator-angular generator-dummy
Findup searches for the first `node_modules` directory in cwd and it's ancestors, but what if our generators in `~/node_modules`, and we calling generator in directory, that has `node_modules`? Right - `yo` will think, that you have 0 generators. Closes yeoman#440
Merged npm installs. |
Thank you a lot for this one! Good work, keep them coming. |
Thanks for advices and code-review @SBoudrias ! I hope next time my code will be clearer. 😊 P.s. this pr will allow |
Findup searches for the first
node_modules
directory in cwd andit's ancestors, but what if our generators in
~/node_modules
,and we calling generator in directory, that has
node_modules
?Right -
yo
will think, that you have 0 generators.Closes #440