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

Error: Cannot find module 'bower' #161

Closed
Turbo87 opened this issue Nov 13, 2017 · 10 comments
Closed

Error: Cannot find module 'bower' #161

Turbo87 opened this issue Nov 13, 2017 · 10 comments
Assignees
Labels

Comments

@Turbo87
Copy link
Member

Turbo87 commented Nov 13, 2017

#157 appears to have broken CI for ember-cli-shims (see https://travis-ci.org/ember-cli/ember-cli-shims/jobs/300982718) and I assume probably for other addons too.

ember-cli-shims is running CI once per day, yesterday it worked and today it is broken with this error:

Error: Cannot find module 'bower' from '/home/travis/build/ember-cli/ember-cli-shims/node_modules/ember-cli/bin'
    at /home/travis/build/ember-cli/ember-cli-shims/node_modules/resolve/lib/async.js:48:31
    at processDirs (/home/travis/build/ember-cli/ember-cli-shims/node_modules/resolve/lib/async.js:182:39)
    at ondir (/home/travis/build/ember-cli/ember-cli-shims/node_modules/resolve/lib/async.js:197:13)
    at load (/home/travis/build/ember-cli/ember-cli-shims/node_modules/resolve/lib/async.js:80:43)
    at onex (/home/travis/build/ember-cli/ember-cli-shims/node_modules/resolve/lib/async.js:105:17)
    at /home/travis/build/ember-cli/ember-cli-shims/node_modules/resolve/lib/async.js:26:73
    at FSReqWrap.oncomplete (fs.js:123:15)

/cc @kategengler @hjdivad @rwjblue @twokul

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 13, 2017

ember-cli-mocha has the same issue: https://travis-ci.org/ember-cli/ember-cli-mocha/jobs/301308777

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 13, 2017

It appears that all scenarios are affected that don't use Bower at all because they rely on ember-source. That means ember-default for most cases and ember-lts-2.12/2.16.

@rwjblue
Copy link
Member

rwjblue commented Nov 13, 2017

Ugh. Thanks for reporting.

Why are we still attempting to resolve bower when it isn’t used for the scenario?

@rwjblue rwjblue self-assigned this Nov 13, 2017
@rwjblue
Copy link
Member

rwjblue commented Nov 13, 2017

I have a repro, working on a solution now.

@Emrvb
Copy link

Emrvb commented Nov 13, 2017

@rwjblue

There are two reasons for that:

  1. Dependency manager adapters are loaded for a task based on all scenarios. So if one scenario uses bower, the adapter is loaded and used for every scenario that will be run.
  2. The bower dependency manager never skips because _getDependencySetAccountingForDeprecatedTopLevelKeys will always make the adapter assume there are dependencies to install.

I've been working on patch as well. Want a diff?

@rwjblue
Copy link
Member

rwjblue commented Nov 13, 2017

@Emrvb:

Dependency manager adapters are loaded for a task based on all scenarios. So if one scenario uses bower, the adapter is loaded and used for every scenario that will be run.

This would only affect the ember try:each scenario, and should essentially still work properly as far as I can tell?

The bower dependency manager never skips because _getDependencySetAccountingForDeprecatedTopLevelKeys will always make the adapter assume there are dependencies to install.

Right, this is the real issue IMHO. We need to tweak changeToDependencySet something like:

    depSet = this._getDependencySetAccountingForDeprecatedTopLevelKeys(depSet);
    var needsInstall = depSet && Object.keys(depSet.dependencies).length + Object.keys(depSet.devDependencies).length > 0;
    if (!needsInstall) { 
      return RSVP.resolve([]); 
    }

@Emrvb
Copy link

Emrvb commented Nov 13, 2017

@rwjblue

This would only affect the ember try:each scenario, and should essentially still work properly as far as I can tell?

Not exactly, the try:one command depends on the try-each-test task, which will always load all scenarios.

Right, this is the real issue IMHO. We need to tweak changeToDependencySet something like:

Since the problem is caused by _getDependencySetAccountingForDeprecatedTopLevelKeys I thought of fixing it like this:

_getDependencySetAccountingForDeprecatedTopLevelKeys: function(depSet) {
    if (depSet[this.configKey]) {
      return depSet[this.configKey];
    } else if ('dependencies' in depSet || 'devDependencies' in depSet || 'resolutions' in depSet) {
      return { dependencies: depSet.dependencies, devDependencies: depSet.devDependencies, resolutions: depSet.resolutions };
    }
    return null;
  },

@hjdivad
Copy link
Contributor

hjdivad commented Nov 13, 2017

Summarizing discussion + fix WIP with @rwjblue

  • load bower adapter all the time
  • bower adapter learn to no-op if it has no dependencies to install
  • add implicit bower dependency for legacy top-level dependencies and devDependencies
  • add implicit bower dependency if there exists a bower.json
  • do not run bower install during cleanup if we no-op because we had no dependencies to install

@kategengler
Copy link
Member

add implicit bower dependency if there exists a bower.json

Why would we want to add a bower dep even when there are not any bower scenarios?

@hjdivad
Copy link
Contributor

hjdivad commented Nov 13, 2017

Why would we want to add a bower dep even when there are not any bower scenarios?

In typing out a reply, I realize that I was wrong on this point.


The case we want to deal with is

There are bower dependencies in scenario A, but not in scenario B.

We were running into problems cleaning up in B, but we should just no-op in that case. We'll still be able to clean up in A because bower will have been installed, since there were bower dependencies.

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

No branches or pull requests

5 participants