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

Fixes Project#hasDependencies to only check for dependencies instead … #7414

Merged

Conversation

MiguelMadero
Copy link
Contributor

@MiguelMadero MiguelMadero commented Nov 2, 2017

…of all dependencies

cc: @rwjblue @ef4

Not sure if this is the best solution, but I see two problems with the current implementation:

  1. If I install with yarn --prod I won't have the devDependencies, which may show up first when running this.dependencies()
  2. Some dependencies can't be resolved like @types/node. I'm not sure how to fix this problem. All of our types are devDependencies, so this indirectly fixed it, but my guess is that there might be other packages that might be valid as a "dependency" that can't be resolved.

@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2017

If I install with yarn --prod I won't have the devDependencies, which may show up first when running this.dependencies()

In general, this isn't something that ember-cli expects to happen out of the box. Notice how all deps added to newly generated apps are devDependencies? This is absolutely intentional, as ember-cli is never intended to be used in production.

Some dependencies can't be resolved like @types/node. I'm not sure how to fix this problem. All of our types are devDependencies, so this indirectly fixed it, but my guess is that there might be other packages that might be valid as a "dependency" that can't be resolved.

We should update hasDependencies to do something like this instead:

this.resolveSync(`${depName}/package`);

This will avoid the issue above because absolutely every package has to have a package.json (or it can't be published).

@@ -132,7 +132,8 @@ class Project {
// Blueprint tests set this flag.
return true;
}
for (let depName in this.dependencies()) {
const excludeDevDeps = true;
for (let depName in this.dependencies(this.pkg, excludeDevDeps)) {
try {
this.resolveSync(depName);
Copy link
Member

Choose a reason for hiding this comment

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

Can you tweak this to use the /package thing I mentioned in another comment? I think that is the only real blocker for this PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll give that a go. Thanks

@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2017

all deps added to newly generated apps are devDependencies

FWIW, this is why CI is failing. As written in this PR ATM Project#hasDependencies() on a newly generated app is returning false (because there are no deps in a new app).

@MiguelMadero
Copy link
Contributor Author

This is absolutely intentional, as ember-cli is never intended to be used in production.

Sounds good. My use case is a bit different since not everything on our mono-repo is an ember app/addon, but I think the check for package should address both. I'll test it and update it.

@MiguelMadero
Copy link
Contributor Author

That worked. I'll wait for the tests before getting too excited, but it worked fine on my local setup. I think this addresses both scenarios and it's a cleaner solution.

@ef4
Copy link
Contributor

ef4 commented Nov 3, 2017

This looks good to me now. 👍

@@ -134,7 +134,7 @@ class Project {
}
for (let depName in this.dependencies()) {
try {
this.resolveSync(depName);
this.resolveSync(depName + '/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.

The manual string concat here is failing linting / CI:


not ok 10 eslint should have no errors in lib
  Error: Code did not pass lint rules
  lib/models/project.js
    137:26  error  Unexpected string concatenation  prefer-template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I saw the failure yesterday, but I didn't get a chance to push a fix. It should be good now.

Some tests failed locally, but I'm not sure if those are related, I'll keep an eye on travis.

…t contain a entry point

Every dependecny should have a package.json so we check for that instead
@rwjblue rwjblue merged commit 4cdb017 into ember-cli:master Nov 3, 2017
@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2017

Thank you!

@MiguelMadero MiguelMadero deleted the mmadero/fixes-hasDependencies branch November 3, 2017 23:58
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.

None yet

4 participants