Repository: ember-cli/ember-cli
Pull request #6043 (2016-07-06 15:20:25)

[BUGFIX beta] Test nested addon import
refs #5990 

This PR updates `Addon.prototype.import` (https://github.com/ember-cli/ember-cli/pull/6043/files#diff-f1e1923c0440a385f51d2a81c0e53b64R448) as per @machty's implementation (see https://github.com/machty/ember-maybe-import-regenerator/blob/master/index.js#L38-L50).

It also adds a smoke test that builds an app with one addon that itself requires 2 more (hence nested) addons. These addons imports a file each, one with the new `this.import(asset)`, the other with the current `app.import(asset)`.

The PR makes sure that independently from the addon developer's preference (or history), the import binds to the correct app, thanks to @machty's implementation.

@Turbo87 Though I let the smoke test at its initial place, I am more than willing to move it to a more appropriate location, your recommandations are welcome.

As a follow up to #5990, ping @rwjblue and @nathanhammond.

Turbo87: @xcambar I just checked that the current `master` branch is still passing on Travis. It seems that these test failures in the PR are real and caused by the proposed changes. I haven't looked at what exactly changed yet but it seems to influence some other code paths.
xcambar: Thanks for investigating. FWIW, I'm currently trying to split the changes into smaller chunks and see when/why it breaks master.
rwjblue: Looks like it is green.  Can you squash down to a single commit, and prefix with `[BUGFIX beta]` to help ensure we pull this down into the beta branch before releasing 2.7?

Patch:
diff --git a/lib/commands/install.js b/lib/commands/install.js
index 0cf3bd8c6..bf123e850 100644
--- a/lib/commands/install.js
+++ b/lib/commands/install.js
@@ -10,11 +10,6 @@ module.exports = Command.extend({
   aliases: ['i'],
   works: 'insideProject',
 
-  availableOptions: [
-    { name: 'save', type: Boolean, default: false, aliases: ['S'] },
-    { name: 'save-dev', type: Boolean, default: true, aliases: ['D'] }
-  ],
-
   anonymousOptions: [
     '<addon-name>'
   ],
diff --git a/lib/models/addon.js b/lib/models/addon.js
index 15e23f925..d9111f978 100644
--- a/lib/models/addon.js
+++ b/lib/models/addon.js
@@ -376,6 +376,31 @@ var Addon = CoreObject.extend({
     return tree;
   },
 
+  /**
+    This method climbs up the hierarchy of addons
+    up to the host application.
+
+    This prevents previous addons (prior to `this.import`, ca 2.7.0)
+    to break at importing assets when they are used nested in other addons.
+
+    @private
+    @method _findHost
+    ```
+  */
+  _findHost: function() {
+    var app = this.app;
+    var parent = this.parent;
+    while (parent.parent) {
+      if (parent.app) {
+        app = parent.app;
+        break;
+      }
+
+      parent = parent.parent;
+    }
+    return app;
+  },
+
   /**
     This method is called when the addon is included in a build. You
     would typically use this hook to perform additional imports
@@ -420,10 +445,7 @@ var Addon = CoreObject.extend({
      @param {String} [options.destDir] Destination directory, defaults to the name of the directory the asset is in
    */
   import: function(asset, options) {
-    var app = this.app;
-    while (app.app) {
-      app = app.app;
-    }
+    var app = this._findHost();
     app.import(asset, options);
   },
 
diff --git a/lib/models/asset-size-printer.js b/lib/models/asset-size-printer.js
index ab09ce6d1..45767bd28 100644
--- a/lib/models/asset-size-printer.js
+++ b/lib/models/asset-size-printer.js
@@ -10,7 +10,6 @@ function AssetPrinterSize(options) {
 }
 
 AssetPrinterSize.prototype.print = function () {
-  var filesize = require('filesize');
   var ui = this.ui;
 
   return this.makeAssetSizesObject().then(function (files) {
@@ -18,9 +17,9 @@ AssetPrinterSize.prototype.print = function () {
 
     ui.writeLine(chalk.green('File sizes:'));
     return files.forEach(function (file) {
-      var sizeOutput = filesize(file.size);
+      var sizeOutput = file.size;
       if (file.showGzipped) {
-        sizeOutput += ' (' + filesize(file.gzipSize) + ' gzipped)';
+        sizeOutput += ' (' + file.gzipSize + ' gzipped)';
       }
 
       ui.writeLine(chalk.blue(' - ' + file.name + ': ') + chalk.white(sizeOutput));
@@ -30,6 +29,7 @@ AssetPrinterSize.prototype.print = function () {
 };
 
 AssetPrinterSize.prototype.makeAssetSizesObject = function () {
+  var filesize = require('filesize');
   var fs = require('fs');
   var zlib = require('zlib');
   var gzip = Promise.denodeify(zlib.gzip);
@@ -49,8 +49,8 @@ AssetPrinterSize.prototype.makeAssetSizesObject = function () {
       return gzip(contentsBuffer).then(function (buffer) {
         return {
           name: filename,
-          size: contentsBuffer.length,
-          gzipSize: buffer.length,
+          size: filesize(contentsBuffer.length),
+          gzipSize: filesize(buffer.length),
           showGzipped: contentsBuffer.length > 0
         };
       });
diff --git a/lib/tasks/addon-install.js b/lib/tasks/addon-install.js
index 4649916b7..f3da9adbd 100644
--- a/lib/tasks/addon-install.js
+++ b/lib/tasks/addon-install.js
@@ -16,9 +16,8 @@ module.exports = Task.extend({
   run: function(options) {
     var chalk            = require('chalk');
     var ui               = this.ui;
-    var packageNames     = options.packages;
+    var packageNames     = options['packages'];
     var blueprintOptions = options.blueprintOptions || {};
-    var commandOptions   = blueprintOptions;
 
     var npmInstall = new this.NpmInstallTask({
       ui:         this.ui,
@@ -36,9 +35,8 @@ module.exports = Task.extend({
     ui.startProgress(chalk.green('Installing addon package'), chalk.green('.'));
 
     return npmInstall.run({
-      'packages': packageNames,
-      'save': commandOptions.save,
-      'save-dev': !commandOptions.save && commandOptions.saveDev,
+      packages: packageNames,
+      'save-dev': true,
       'save-exact': true
     }).then(function() {
       return this.project.reloadAddons();
diff --git a/lib/tasks/npm-task.js b/lib/tasks/npm-task.js
index e997d8a84..3c5354b6d 100644
--- a/lib/tasks/npm-task.js
+++ b/lib/tasks/npm-task.js
@@ -28,7 +28,6 @@ module.exports = Task.extend({
       color: 'always',
       // by default, do install peoples optional deps
       'optional': 'optional' in options ? options.optional : true,
-      'save': !!options.save,
       'save-dev': !!options['save-dev'],
       'save-exact': !!options['save-exact']
     };


Test patch:
diff --git a/tests/acceptance/nested-addons-smoke-test-slow.js b/tests/acceptance/nested-addons-smoke-test-slow.js
new file mode 100644
index 000000000..7cbba802d
--- /dev/null
+++ b/tests/acceptance/nested-addons-smoke-test-slow.js
@@ -0,0 +1,59 @@
+'use strict';
+
+var path       = require('path');
+var fs         = require('fs-extra');
+
+var runCommand          = require('../helpers/run-command');
+var acceptance          = require('../helpers/acceptance');
+var copyFixtureFiles    = require('../helpers/copy-fixture-files');
+var createTestTargets   = acceptance.createTestTargets;
+var teardownTestTargets = acceptance.teardownTestTargets;
+var linkDependencies    = acceptance.linkDependencies;
+var cleanupRun          = acceptance.cleanupRun;
+
+var chai = require('../chai');
+var expect = chai.expect;
+var file = chai.file;
+var dir = chai.dir;
+
+var appName  = 'some-cool-app';
+
+describe('Acceptance: nested-addons-smoke-test', function() {
+  this.timeout(360000);
+
+  before(function() {
+    return createTestTargets(appName);
+  });
+
+  after(function() {
+    return teardownTestTargets();
+  });
+
+  beforeEach(function() {
+    return linkDependencies(appName);
+  });
+
+  afterEach(function() {
+    return cleanupRun(appName).then(function() {
+      expect(dir('tmp/' + appName)).to.not.exist;
+    });
+  });
+
+  it('addons with nested addons compile correctly', function() {
+    return copyFixtureFiles('addon/with-nested-addons')
+      .then(function() {
+        var packageJsonPath = path.join(__dirname, '..', '..', 'tmp', appName, 'package.json');
+        var packageJson = fs.readJsonSync(packageJsonPath);
+        packageJson.devDependencies['ember-top-addon'] = 'latest';
+
+        return fs.writeJsonSync(packageJsonPath, packageJson);
+      })
+      .then(function() {
+        return runCommand(path.join('.', 'node_modules', 'ember-cli', 'bin', 'ember'), 'build');
+      })
+      .then(function() {
+        expect(file('dist/assets/vendor.js')).to.contain('INNER_ADDON_IMPORT_WITH_APP_IMPORT');
+        expect(file('dist/assets/vendor.js')).to.contain('INNER_ADDON_IMPORT_WITH_THIS_IMPORT');
+      });
+  });
+});
diff --git a/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/index.js b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/index.js
new file mode 100644
index 000000000..ed2a7848c
--- /dev/null
+++ b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/index.js
@@ -0,0 +1,6 @@
+'use strict';
+
+module.exports = {
+  name: 'ember-top-addon'
+};
+
diff --git a/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/index.js b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/index.js
new file mode 100644
index 000000000..6d3c93dd2
--- /dev/null
+++ b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/index.js
@@ -0,0 +1,8 @@
+'use strict';
+
+module.exports = {
+  name: 'Ember Inner Addon',
+  included: function(app) {
+    app.import('vendor/inner.js');
+  }
+};
diff --git a/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/package.json b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/package.json
new file mode 100644
index 000000000..1cebfe85d
--- /dev/null
+++ b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/package.json
@@ -0,0 +1,12 @@
+{
+  "name": "ember-inner-addon",
+  "version": "0.0.0",
+  "private": true,
+  "keywords": [
+    "ember-addon"
+  ],
+  "devDependencies": {
+    "loader.js": "latest"
+  }
+}
+
diff --git a/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/vendor/inner.js b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/vendor/inner.js
new file mode 100644
index 000000000..072e547f7
--- /dev/null
+++ b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon/vendor/inner.js
@@ -0,0 +1,1 @@
+//INNER_ADDON_IMPORT_WITH_APP_IMPORT
diff --git a/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/index.js b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/index.js
new file mode 100644
index 000000000..bb3949c4a
--- /dev/null
+++ b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/index.js
@@ -0,0 +1,8 @@
+'use strict';
+
+module.exports = {
+  name: 'Ember Inner Addon 2',
+  included: function() {
+    this.import('vendor/inner2.js');
+  }
+};
diff --git a/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/package.json b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/package.json
new file mode 100644
index 000000000..3053a3ae9
--- /dev/null
+++ b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/package.json
@@ -0,0 +1,12 @@
+{
+  "name": "ember-inner-addon2",
+  "version": "0.0.0",
+  "private": true,
+  "keywords": [
+    "ember-addon"
+  ],
+  "devDependencies": {
+    "loader.js": "latest"
+  }
+}
+
diff --git a/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/vendor/inner2.js b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/vendor/inner2.js
new file mode 100644
index 000000000..f04a321e2
--- /dev/null
+++ b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/node_modules/ember-inner-addon2/vendor/inner2.js
@@ -0,0 +1,1 @@
+//INNER_ADDON_IMPORT_WITH_THIS_IMPORT
diff --git a/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/package.json b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/package.json
new file mode 100644
index 000000000..8b82da4d6
--- /dev/null
+++ b/tests/fixtures/addon/with-nested-addons/node_modules/ember-top-addon/package.json
@@ -0,0 +1,12 @@
+{
+  "name": "ember-top-addon",
+  "version": "0.0.0",
+  "private": true,
+  "keywords": [
+    "ember-addon"
+  ],
+  "dependencies": {
+    "ember-inner-addon": "latest",
+    "ember-inner-addon2": "latest"
+  }
+}
diff --git a/tests/fixtures/help/help-with-addon.txt b/tests/fixtures/help/help-with-addon.txt
index b3583fb5e..15c0ad5ba 100644
--- a/tests/fixtures/help/help-with-addon.txt
+++ b/tests/fixtures/help/help-with-addon.txt
@@ -90,13 +90,9 @@ ember init \u001b[33m<glob-pattern>\u001b[39m \u001b[36m<options...>\u001b[39m
   \u001b[36m--name\u001b[39m \u001b[36m(String)\u001b[39m \u001b[36m(Default: )\u001b[39m
     \u001b[90maliases: -n <value>\u001b[39m
 
-ember install \u001b[33m<addon-name>\u001b[39m \u001b[36m<options...>\u001b[39m
+ember install \u001b[33m<addon-name>\u001b[39m
   Installs an ember-cli addon from npm.
   \u001b[90maliases: i\u001b[39m
-  \u001b[36m--save\u001b[39m \u001b[36m(Boolean)\u001b[39m \u001b[36m(Default: false)\u001b[39m
-    \u001b[90maliases: -S\u001b[39m
-  \u001b[36m--save-dev\u001b[39m \u001b[36m(Boolean)\u001b[39m \u001b[36m(Default: true)\u001b[39m
-    \u001b[90maliases: -D\u001b[39m
 
 ember new \u001b[33m<app-name>\u001b[39m \u001b[36m<options...>\u001b[39m
   Creates a new directory and runs \u001b[32member init\u001b[39m in it.
diff --git a/tests/fixtures/help/help.js b/tests/fixtures/help/help.js
index 64ee93749..128f78073 100644
--- a/tests/fixtures/help/help.js
+++ b/tests/fixtures/help/help.js
@@ -386,22 +386,7 @@ module.exports = {
       description: 'Installs an ember-cli addon from npm.',
       aliases: ['i'],
       works: 'insideProject',
-      availableOptions: [
-        {
-          name: 'save',
-          default: false,
-          aliases: ['S'],
-          key: 'save',
-          required: false
-        },
-        {
-          name: 'save-dev',
-          default: true,
-          aliases: ['D'],
-          key: 'saveDev',
-          required: false
-        }
-      ],
+      availableOptions: [],
       anonymousOptions: ['<addon-name>']
     },
     {
diff --git a/tests/fixtures/help/help.txt b/tests/fixtures/help/help.txt
index c74e740bd..743cd2a0c 100644
--- a/tests/fixtures/help/help.txt
+++ b/tests/fixtures/help/help.txt
@@ -90,13 +90,9 @@ ember init \u001b[33m<glob-pattern>\u001b[39m \u001b[36m<options...>\u001b[39m
   \u001b[36m--name\u001b[39m \u001b[36m(String)\u001b[39m \u001b[36m(Default: )\u001b[39m
     \u001b[90maliases: -n <value>\u001b[39m
 
-ember install \u001b[33m<addon-name>\u001b[39m \u001b[36m<options...>\u001b[39m
+ember install \u001b[33m<addon-name>\u001b[39m
   Installs an ember-cli addon from npm.
   \u001b[90maliases: i\u001b[39m
-  \u001b[36m--save\u001b[39m \u001b[36m(Boolean)\u001b[39m \u001b[36m(Default: false)\u001b[39m
-    \u001b[90maliases: -S\u001b[39m
-  \u001b[36m--save-dev\u001b[39m \u001b[36m(Boolean)\u001b[39m \u001b[36m(Default: true)\u001b[39m
-    \u001b[90maliases: -D\u001b[39m
 
 ember new \u001b[33m<app-name>\u001b[39m \u001b[36m<options...>\u001b[39m
   Creates a new directory and runs \u001b[32member init\u001b[39m in it.
diff --git a/tests/fixtures/help/with-addon-blueprints.js b/tests/fixtures/help/with-addon-blueprints.js
index c7660abb1..322feb362 100644
--- a/tests/fixtures/help/with-addon-blueprints.js
+++ b/tests/fixtures/help/with-addon-blueprints.js
@@ -418,22 +418,7 @@ module.exports = {
       description: 'Installs an ember-cli addon from npm.',
       aliases: ['i'],
       works: 'insideProject',
-      availableOptions: [
-        {
-          name: 'save',
-          default: false,
-          aliases: ['S'],
-          key: 'save',
-          required: false
-        },
-        {
-          name: 'save-dev',
-          default: true,
-          aliases: ['D'],
-          key: 'saveDev',
-          required: false
-        }
-      ],
+      availableOptions: [],
       anonymousOptions: ['<addon-name>']
     },
     {
diff --git a/tests/fixtures/help/with-addon-commands.js b/tests/fixtures/help/with-addon-commands.js
index 82be7e949..eddc55ae0 100644
--- a/tests/fixtures/help/with-addon-commands.js
+++ b/tests/fixtures/help/with-addon-commands.js
@@ -386,22 +386,7 @@ module.exports = {
       description: 'Installs an ember-cli addon from npm.',
       aliases: ['i'],
       works: 'insideProject',
-      availableOptions: [
-        {
-          name: 'save',
-          default: false,
-          aliases: ['S'],
-          key: 'save',
-          required: false
-        },
-        {
-          name: 'save-dev',
-          default: true,
-          aliases: ['D'],
-          key: 'saveDev',
-          required: false
-        }
-      ],
+      availableOptions: [],
       anonymousOptions: ['<addon-name>']
     },
     {
diff --git a/tests/unit/commands/install-test.js b/tests/unit/commands/install-test.js
index 15b74267d..a558d8425 100644
--- a/tests/unit/commands/install-test.js
+++ b/tests/unit/commands/install-test.js
@@ -99,64 +99,12 @@ describe('install command', function() {
 
         td.verify(npmRun({
           packages: ['ember-data'],
-          'save': false,
           'save-dev': true,
           'save-exact': true
         }), {times: 1});
       });
     });
 
-    it('runs the npm install task with given name and save-dev true in an addon', function() {
-      command.project.isEmberCLIProject = function() {
-        return false;
-      };
-      command.project.isEmberCLIAddon = function() {
-        return true;
-      };
-      return command.validateAndRun(['ember-data']).then(function() {
-        var npmRun = tasks.NpmInstall.prototype.run;
-
-        td.verify(npmRun({
-          packages: ['ember-data'],
-          'save': false,
-          'save-dev': true,
-          'save-exact': true
-        }), {times: 1});
-      });
-    });
-
-    it('runs the npm install task with given name and save true with the --save option', function() {
-      return command.validateAndRun(['ember-data', '--save']).then(function() {
-        var npmRun = tasks.NpmInstall.prototype.run;
-
-        td.verify(npmRun({
-          packages: ['ember-data'],
-          'save': true,
-          'save-dev': false,
-          'save-exact': true
-        }), {times: 1});
-      });
-    });
-
-    it('runs the npm install task with given name and save true in an addon with the --save option', function() {
-      command.project.isEmberCLIProject = function() {
-        return false;
-      };
-      command.project.isEmberCLIAddon = function() {
-        return true;
-      };
-      return command.validateAndRun(['ember-data', '--save']).then(function() {
-        var npmRun = tasks.NpmInstall.prototype.run;
-
-        td.verify(npmRun({
-          packages: ['ember-data'],
-          'save': true,
-          'save-dev': false,
-          'save-exact': true
-        }), {times: 1});
-      });
-    });
-
     it('runs the package name blueprint task with given name and args', function() {
       return command.validateAndRun(['ember-data']).then(function() {
         var generateRun = tasks.GenerateFromBlueprint.prototype.run;
@@ -195,7 +143,6 @@ describe('install command', function() {
 
         td.verify(npmRun({
           packages: ['ember-data', 'ember-cli-cordova', 'ember-cli-qunit'],
-          'save': false,
           'save-dev': true,
           'save-exact': true
         }), {times: 1});
@@ -212,7 +159,6 @@ describe('install command', function() {
 
         td.verify(npmRun({
           packages: ['ember-cli/ember-cli-qunit'],
-          'save': false,
           'save-dev': true,
           'save-exact': true
         }), {times: 1});
@@ -228,7 +174,6 @@ describe('install command', function() {
 
         td.verify(npmRun({
           packages: ['ember-cli-qunit@1.2.0'],
-          'save': false,
           'save-dev': true,
           'save-exact': true
         }), {times: 1});
@@ -245,7 +190,6 @@ describe('install command', function() {
 
         td.verify(npmRun({
           packages: ['@ember-cli/ember-cli-qunit'],
-          'save': false,
           'save-dev': true,
           'save-exact': true
         }), {times: 1});