Repository: ember-cli/ember-try
Pull request #138 (2017-07-27 15:50:26)

Change Yarn detection to look for lockfile
This is a naïve fix for #133. It would seemingly work to merge, but I’m not sure if more nuance is needed, or whether the change should extend further.

1. I made use of `throw` because it was the minimal change, since that’s what `execFileSync` would do when `yarn --version` failed. I could instead strip out the exception handling and directly use the simple boolean check of `isFile()`.

2. The existing test for this doesn’t really exercise things fully, it mocks `_runYarnCheck`. Would it be worth using a true `yarn.lock` for testing?

3. What happens if a `yarn.lock` is detected but `yarn` itself isn’t present? Should the `execFileSync` perhaps remain? 🤔

Let me know your thoughts and I’m happy to improve this. Thanks for `ember-try`, it’s endlessly useful!

kategengler: Re: 1, I think removing the exception handling would be a nice cleanup if you have the time

Re: 2, Another test with an actual file for yarn.lock is a nice-to-have

Re: 3, I think failing if yarn is not installed but yarn.lock is committed is fine, but we need to add an "out" for people who for some reason have a yarn.lock but would like to use npm. A configuration option to force use of npm or yarn, maybe. (But I wouldn't necessarily block this PR on that)
kellyselden: Can this check for yarn installed as well as yarn.lock? That way CI servers that don't have yarn won't start breaking with this change.
kategengler: @backspace Thanks for working on this. After thinking it through and chatting with @kellyselden and @rwjblue, we'd like to make use of yarn be only through opt-in via the config (`useYarn: true`). We'll also warn if the user has a yarn.lock and yarn on the system and does not have useYarn: true in the config (but only if there's a scenario with npm dependencies). 

Are you interested/willing/do have time to take this PR in that direction?
backspace: I’m into replacing this with the plan you detailed. Do you think that maybe the presence of `yarn.lock` alone without `useYarn: true` should suggest `useYarn` (regardless of whether `yarn` exists as a command), since presumably if they have the file they probably want to use it and should make sure it’s installed?

I should be able to try getting into this direction later this weekend or during the upcoming week.

I did have a [mysterious CI failure](https://travis-ci.org/ember-cli/ember-try/jobs/260833733#L3579-L3580) in my work on this to date:

```
  1) tryEach with stubbed dependency manager passes along timeout options to run:
     Error: timeout of 1200ms exceeded. Ensure the done() callback is being called in this test.
```

There’s a comment within the test specifically addressing it:

```
        // With stubbed dependency manager, timing out is warning for accidentally not using the stub
```

It was confusing to me because I didn’t think I had changed anything related to using the stub. But hopefully this error will disappear as I change things around with the current plan 🤞
kategengler: I think that approach w/ just yarn.lock is reasonable, since it'll just be a warning.

Re: CI failure, _most_ of the time that comment is true. I haven't seen that test be flaky before, but it's possible. If it doesn't go away, I'll look into it more.
backspace: > Re: CI failure, _most_ of the time that comment is true. I haven't seen that test be flaky before, but it's possible. If it doesn't go away, I'll look into it more.

hah I totally believe it, I probably changed something I didn’t realise would affect it.

Regarding this:

> we'd like to make use of yarn be only through opt-in via the config (`useYarn: true`)

Am I correct in understanding that this is in the `ember-try.js` file, like if it were in the dummy one:

```
module.exports = {
  scenarios: [
    {
      name: 'test1',
      bower: {
        dependencies: {
          ember: '1.10.0'
        }
      }
    },
    {
      name: 'test2',
      command: 'ember test',
      npm: {
        useYarn: true,
        dependencies: {
          'ember-feature-flags': '3.0.0'
        }
      }
    }
  ]
};
```

Or is it meant to go elsewhere?
backspace: ah, makes sense, thank you!

Patch:
diff --git a/README.md b/README.md
index ed9d2f3..935d9e2 100644
--- a/README.md
+++ b/README.md
@@ -109,84 +109,83 @@ Configuration will be read from a file in your ember app in `config/ember-try.js
 
 module.exports = function() {
   return {
-    /*
-      `command` - a single command that, if set, will be the default command used by `ember-try`.
-      P.S. The command doesn't need to be an `ember <something>` command, they can be anything.
-      Keep in mind that this config file is JavaScript, so you can code in here to determine the command.
-    */
-    command: 'ember test --reporter xunit',
-    /*
-      `bowerOptions` - options to be passed to `bower`.
-    */
-    bowerOptions: ['--allow-root=true'],
-    /*
-      `npmOptions` - options to be passed to `npm`.
-    */
-    npmOptions: ['--loglevel=silent', '--no-shrinkwrap=true'],
-    /*
-      If set to true, the `versionCompatibility` key under `ember-addon` in `package.json` will be used to
-      automatically generate scenarios that will deep merge with any in this configuration file.
-    */
-    useVersionCompatibility: true,
-    scenarios: [
-      {
-        name: 'Ember 1.10 with ember-data',
-
-        /*
-          `command` can also be overridden at the scenario level.
-        */
-        command: 'ember test --filter ember-1-10',
-        bower: {
-          dependencies: {
-            'ember': '1.10.0',
-            'ember-data': '1.0.0-beta.15'
-          }
-        },
+  /*
+    `command` - a single command that, if set, will be the default command used by `ember-try`.
+    P.S. The command doesn't need to be an `ember <something>` command, they can be anything.
+    Keep in mind that this config file is JavaScript, so you can code in here to determine the command.
+  */
+  command: 'ember test --reporter xunit',
+  /*
+    `bowerOptions` - options to be passed to `bower`.
+  */
+  bowerOptions: ['--allow-root=true'],
+  /*
+    `npmOptions` - options to be passed to `npm`.
+  */
+  npmOptions: ['--loglevel=silent', '--no-shrinkwrap=true'],
+  /*
+    If set to true, the `versionCompatibility` key under `ember-addon` in `package.json` will be used to
+    automatically generate scenarios that will deep merge with any in this configuration file.
+  */
+  useVersionCompatibility: true,
+  scenarios: [
+    {
+      name: 'Ember 1.10 with ember-data',
+
+      /*
+        `command` can also be overridden at the scenario level.
+      */
+      command: 'ember test --filter ember-1-10',
+      bower: {
+        dependencies: {
+          'ember': '1.10.0',
+          'ember-data': '1.0.0-beta.15'
+        }
       },
-      {
-        name: 'Ember 1.11.0-beta.5',
-        bower: {
-          dependencies: {
-            'ember': '1.11.0-beta.5'
-          }
+    },
+    {
+      name: 'Ember 1.11.0-beta.5',
+      bower: {
+        dependencies: {
+          'ember': '1.11.0-beta.5'
+        }
+      }
+    },
+    {
+      name: 'Ember canary with Ember-Data 2.3.0',
+      /*
+        `allowedToFail` - If true, if this scenario fails it will not fail the entire try command.
+      */
+      allowedToFail: true,
+      npm: {
+        devDependencies: {
+          'ember-data': '2.3.0',
+
+          // you can remove any package by marking `null`
+          'some-optional-package': null
         }
       },
-      {
-        name: 'Ember canary with Ember-Data 2.3.0',
-        /*
-          `allowedToFail` - If true, if this scenario fails it will not fail the entire try command.
-        */
-        allowedToFail: true,
-        npm: {
-          devDependencies: {
-            'ember-data': '2.3.0',
-
-            // you can remove any package by marking `null`
-            'some-optional-package': null
-          }
+      bower: {
+        dependencies: {
+          'ember': 'components/ember#canary'
         },
-        bower: {
-          dependencies: {
-            'ember': 'components/ember#canary'
-          },
-          resolutions: {
-            'ember': 'canary'
-          }
+        resolutions: {
+          'ember': 'canary'
         }
-      },
-      {
-        name: 'Ember beta',
-        bower: {
-          dependencies: {
-            'ember': 'components/ember#beta'
-          },
-          resolutions: { // Resolutions are only necessary when they do not match the version specified in `dependencies`
-            'ember': 'beta'
-          }
+      }
+    },
+    {
+      name: 'Ember beta',
+      bower: {
+        dependencies: {
+          'ember': 'components/ember#beta'
+        },
+        resolutions: { // Resolutions are only necessary when they do not match the version specified in `dependencies`
+          'ember': 'beta'
         }
       }
-    ]
-  };
+    }
+  ]};
 };
 ```
 
@@ -236,7 +235,7 @@ If no `config/ember-try.js` file is present, the default config will be used. Th
 
 ##### A note on npm scenarios / "What about yarn?"
 
-If `yarn` is available globally on the system the `ember-try` command is run on, all npm scenarios will use `yarn` for install with the `--no-lockfile` option. 
+If you include `useYarn: true` in your `ember-try` config, all npm scenarios will use `yarn` for install with the `--no-lockfile` option.
 If the project has a `yarn.lock`, it will be used *only* on cleanup to restore the project to a clean state. 
  
 ###### But I use yarn, why `--no-lockfile`?
diff --git a/lib/dependency-manager-adapters/npm.js b/lib/dependency-manager-adapters/npm.js
index 39c261d..31727ee 100644
--- a/lib/dependency-manager-adapters/npm.js
+++ b/lib/dependency-manager-adapters/npm.js
@@ -7,13 +7,12 @@ var path = require('path');
 var extend = require('extend');
 var debug = require('debug')('ember-try:dependency-manager-adapter:npm');
 var rimraf = RSVP.denodeify(require('rimraf'));
-var execFileSync = require('child_process').execFileSync;
+var chalk = require('chalk');
 
 module.exports = CoreObject.extend({
   init: function() {
     this._super.apply(this, arguments);
     this.run = this.run || require('../utils/run');
-    this._setYarnAvailability();
   },
   useYarnCommand: false,
   yarnLock: 'yarn.lock',
@@ -22,7 +21,11 @@ module.exports = CoreObject.extend({
   packageJSONBackupFileName: 'package.json.ember-try',
   nodeModules: 'node_modules',
   nodeModulesBackupLocation: '.node_modules.ember-try',
-  setup: function() {
+  setup: function(options) {
+    if (!options) {
+      options = {};
+    }
+    this._runYarnCheck(options.ui);
     return this._backupOriginalDependencies();
   },
   changeToDependencySet: function(depSet) {
@@ -70,19 +73,17 @@ module.exports = CoreObject.extend({
       console.log('Error cleaning up npm scenario:', e);
     });
   },
-  _setYarnAvailability: function() {
-    try {
-      this._runYarnCheck();
-      this.useYarnCommand = true;
-      debug('Using yarn as dependency manager');
-    } catch (e) {
-      this.useYarnCommand = false;
-      debug('Using npm as dependency manager');
+  _runYarnCheck: function(ui) {
+    if (!this.useYarnCommand) {
+      try {
+        if (fs.statSync(path.join(this.cwd, this.yarnLock)).isFile()) {
+          ui.writeLine(chalk.yellow('Detected a yarn.lock file, add useYarn: true to your configuration if you want to use Yarn to install npm dependencies.'));
+        }
+      } catch (e) {
+        // If no yarn.lock is found, no need to warn.
+      }
     }
   },
-  _runYarnCheck: function() {
-    execFileSync('yarn', [].concat(['--version']));
-  },
   _findCurrentVersionOf: function(packageName) {
     var filename = path.join(this.cwd, this.nodeModules, packageName, this.packageJSON);
     if (fs.existsSync(filename)) {
diff --git a/lib/utils/dependency-manager-adapter-factory.js b/lib/utils/dependency-manager-adapter-factory.js
index 75e74c7..6ece4bb 100644
--- a/lib/utils/dependency-manager-adapter-factory.js
+++ b/lib/utils/dependency-manager-adapter-factory.js
@@ -21,7 +21,7 @@ module.exports = {
       }
     });
     if (hasNpm) {
-      adapters.push(new NpmAdapter({ cwd: root, managerOptions: config.npmOptions }));
+      adapters.push(new NpmAdapter({ cwd: root, managerOptions: config.npmOptions, useYarnCommand: config.useYarn }));
     }
     if (hasBower) {
       adapters.push(new BowerAdapter({ cwd: root, managerOptions: config.bowerOptions }));
diff --git a/lib/utils/scenario-manager.js b/lib/utils/scenario-manager.js
index 40db315..4b7df0c 100644
--- a/lib/utils/scenario-manager.js
+++ b/lib/utils/scenario-manager.js
@@ -17,8 +17,10 @@ module.exports = CoreObject.extend({
   },
 
   setup: function() {
+    var ui = this.ui;
+
     return mapSeries(this.dependencyManagerAdapters, function(depManager) {
-      return depManager.setup();
+      return depManager.setup({ ui: ui });
     });
   },
 


Test patch:
diff --git a/test/dependency-manager-adapters/npm-adapter-test.js b/test/dependency-manager-adapters/npm-adapter-test.js
index 887299c..c086cd4 100644
--- a/test/dependency-manager-adapters/npm-adapter-test.js
+++ b/test/dependency-manager-adapters/npm-adapter-test.js
@@ -15,12 +15,6 @@ var root = process.cwd();
 var tmproot = path.join(root, 'tmp');
 var tmpdir;
 
-var failedYarnCheck = function() {
-  throw new Error('Yarn not available');
-};
-
-var passedYarnCheck = function() {};
-
 describe('npmAdapter', function() {
   beforeEach(function() {
     tmpdir = tmp.in(tmproot);
@@ -38,8 +32,7 @@ describe('npmAdapter', function() {
       writeJSONFile('node_modules/prove-it.json', { originalNodeModules: true });
       writeJSONFile('package.json', { originalPackageJSON: true });
       return new NpmAdapter({
-        cwd: tmpdir,
-        _runYarnCheck: failedYarnCheck
+        cwd: tmpdir
       }).setup().then(function() {
         assertFileContainsJSON('package.json.ember-try', { originalPackageJSON: true });
         assertFileContainsJSON('.node_modules.ember-try/prove-it.json', { originalNodeModules: true });
@@ -73,8 +66,7 @@ describe('npmAdapter', function() {
 
         return new NpmAdapter({
           cwd: tmpdir,
-          run: stubbedRun,
-          _runYarnCheck: failedYarnCheck
+          run: stubbedRun
         })._install().then(function() {
           expect(runCount).to.equal(2, 'Both commands should run');
         }).catch(function(err) {
@@ -103,7 +95,6 @@ describe('npmAdapter', function() {
         return new NpmAdapter({
           cwd: tmpdir,
           run: stubbedRun,
-          _runYarnCheck: failedYarnCheck,
           managerOptions: ['--no-shrinkwrap=true']
         })._install().then(function() {
           expect(runCount).to.equal(2, 'Both commands should run');
@@ -129,7 +120,7 @@ describe('npmAdapter', function() {
         return new NpmAdapter({
           cwd: tmpdir,
           run: stubbedRun,
-          _runYarnCheck: passedYarnCheck
+          useYarnCommand: true
         })._install().then(function() {
           expect(runCount).to.equal(1, 'Only yarn install should run');
         });
@@ -149,7 +140,7 @@ describe('npmAdapter', function() {
         return new NpmAdapter({
           cwd: tmpdir,
           run: stubbedRun,
-          _runYarnCheck: passedYarnCheck,
+          useYarnCommand: true,
           managerOptions: ['--flat']
         })._install().then(function() {
           expect(runCount).to.equal(1, 'Only yarn install should run with manager options');
@@ -215,32 +206,6 @@ describe('npmAdapter', function() {
       expect(resultJSON.devDependencies).to.not.have.property('ember-feature-flags');
     });
   });
-
-  describe('#_setYarnAvailability', function() {
-    it('sets useYarnCommand to true if yarn check does not raise', function() {
-      var npmAdapter = new NpmAdapter({
-        cwd: tmpdir,
-        _runYarnCheck: function() {}
-      });
-
-      npmAdapter._setYarnAvailability();
-
-      expect(npmAdapter.useYarnCommand).to.equal(true);
-    });
-
-    it('sets useYarnCommand to false if yarn check raises', function() {
-      var npmAdapter = new NpmAdapter({
-        cwd: tmpdir,
-        _runYarnCheck: function() {
-          throw new Error();
-        }
-      });
-
-      npmAdapter._setYarnAvailability();
-
-      expect(npmAdapter.useYarnCommand).to.equal(false);
-    });
-  });
 });
 
 function assertFileContainsJSON(filename, expectedObj) {
diff --git a/test/fixtures/dummy-ember-try-config-with-npm-scenarios.js b/test/fixtures/dummy-ember-try-config-with-npm-scenarios.js
index 4a6cf41..b0f93ed 100644
--- a/test/fixtures/dummy-ember-try-config-with-npm-scenarios.js
+++ b/test/fixtures/dummy-ember-try-config-with-npm-scenarios.js
@@ -1,4 +1,5 @@
 module.exports = {
+  useYarn: true,
   scenarios: [
     {
       name: 'test1',
diff --git a/test/tasks/try-each-test.js b/test/tasks/try-each-test.js
index 5863145..57050d7 100644
--- a/test/tasks/try-each-test.js
+++ b/test/tasks/try-each-test.js
@@ -264,10 +264,12 @@ describe('tryEach', function() {
       });
 
       writeJSONFile('package.json', fixturePackage);
+      fs.writeFileSync('yarn.lock', '');
       fs.mkdirSync('node_modules');
       writeJSONFile('bower.json', fixtureBower);
       return tryEachTask.run(config.scenarios, {}).then(function(exitCode) {
         expect(exitCode).to.equal(0, 'exits 0 when all scenarios succeed');
+        expect(output).to.include('Detected a yarn.lock file, add useYarn: true to your configuration if you want to use Yarn to install npm dependencies.');
         expect(output).to.include('Scenario first: SUCCESS');
         expect(output).to.include('Scenario second: SUCCESS');
         expect(output).to.include('Scenario with-bower-resolutions: SUCCESS');