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

Support npm packages as ember new blueprints #6789

Merged
merged 3 commits into from
Feb 20, 2017

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Feb 19, 2017

Closes #6772

  • tries to install from NPM if a blueprint with a matching name is not found
  • only works for ember new and ember addon
  • fails if the npm package does not have the ember-blueprint keyword
  • hidden behind NPM_BLUEPRINTS experiment flag
  • has tests 😉

@Turbo87 Turbo87 changed the title Npm blueprints Support npm packages as ember new blueprints Feb 19, 2017
@@ -80,6 +113,30 @@ class InstallBlueprintTask extends Task {
return execa('npm', ['install'], { cwd });
}

_npmInstallModule(module, cwd) {
logger.info(`Running "npm install ${module}" in "${cwd}" ...`);
return execa('npm', ['install', module], { cwd })
Copy link
Member

Choose a reason for hiding this comment

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

Should use yarn if present (or based on the same rules as #6748).

Copy link
Member Author

Choose a reason for hiding this comment

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

@rwjblue I would like to do something like that in a separate follow-up PR. as you can see a few lines above the existing code of this feature already uses npm exclusively.

Copy link
Member

Choose a reason for hiding this comment

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

gotcha, seems good, can you either confirm this is encompassed by an existing issue or create a new issue (so that we don't loose track)?

Copy link
Member Author

Choose a reason for hiding this comment

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

_npmInstallModule(module, cwd) {
logger.info(`Running "npm install ${module}" in "${cwd}" ...`);
return execa('npm', ['install', module], { cwd })
.then(() => path.join(cwd, 'node_modules', module));
Copy link
Member

Choose a reason for hiding this comment

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

Should we use path.dirname(resolve('module/package', { baseDir: cwd })? I'm unsure there are any cases where what you have now is not correct though, feel free to disregard...

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's necessary in this case


_validateNpmModule(modulePath, packageName) {
logger.info(`Checking for "ember-blueprint" keyword in "${packageName}" module ...`);
let pkg = require(path.join(modulePath, 'package.json'));
Copy link
Member

Choose a reason for hiding this comment

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

require(path.join(modulePath), 'package') should be enough (no need to include extension for require).

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, but it's probably faster since the list of extensions won't have to be tried sequentially, right?

Copy link
Member

@rwjblue rwjblue Feb 19, 2017

Choose a reason for hiding this comment

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

True, though speed probably isn't a concern here. As I read this again though, I actually think what you have is easier to grok than what I wrote anyways.

I can see my future self saying "WTF is ${modulePath}/package?" 😝 (whereas ${modulePath}/package.json actually makes it clearer)

@Turbo87
Copy link
Member Author

Turbo87 commented Feb 20, 2017

@homu r+

@homu
Copy link
Contributor

homu commented Feb 20, 2017

📌 Commit e406454 has been approved by Turbo87

@homu
Copy link
Contributor

homu commented Feb 20, 2017

⚡ Test exempted - status

@homu homu merged commit e406454 into ember-cli:master Feb 20, 2017
homu added a commit that referenced this pull request Feb 20, 2017
Support npm packages as `ember new` blueprints

Closes #6772

- [x] tries to install from NPM if a blueprint with a matching name is not found
- [x] only works for `ember new` and `ember addon`
- [x] fails if the npm package does not have the `ember-blueprint` keyword
- [x] hidden behind `NPM_BLUEPRINTS` experiment flag
- [x] has tests 😉
@Turbo87 Turbo87 deleted the npm-blueprints branch February 20, 2017 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants