Repository: ember-cli/ember-cli
Pull request #6017 (2016-06-23 10:43:19)

Allow `ember install addon_name --save` in addons.
My team and I are regularly annoyed when developing addons with the following scenario:

* execute `ember install whatever_addon` in the addon `our_addon`
* execute `ember install `our_addon` in the app `our_app`
* Realise that `whatever addon` is not packaged in the app `our_app`
* Cringe
* Remind ourselves why it's a perfectly sane situation
* Update `package.json` in `our_addon` to move `whatever_addon` from `devDependencies` to `dependencies`
* Rinse
* Repeat

This PR add the option `--save` to `ember install` that saves addons in `dependencies` instead of `devDependencies` iff the command is run in an addon.

Hope you'll like it!

rwjblue: I have also been trolled by this in the past. I'm in favor. 
rwjblue: I'm unsure why Travis builds didn't kick off though, perhaps a rebase and force push will trigger them?
Turbo87: IMHO `--save` is too close to `npm install --save`, which has different semantics if you don't use that flag. I'd prefer something like `--no-dev` maybe.
rwjblue: How are the semantics different? npm install --save adds a given thing to the deps instead of devDeps. What we do now by default is equivalent to '--save-dev -E'.
Turbo87: if `ember install === npm install --save-dev` then `ember install --save === npm install --save-dev --save` but what is the behavior for having both flags?

or in other words: to me `--save` means "save to the default place in package.json" and `--save-dev` means "save specifically to `devDependencies`". the proposed `--save` however means "save specifically to `dependencies`", so I think conceptually there is a difference here.
rwjblue: > to me --save means "save to the default place in package.json"

Right, I think this is the main disconnect.  This mental model happens to work (since the default place for dependencies is `dependencies` and not `devDependencies`), but I do not believe it to be correct. `--save` means save to `dependencies` and is not about a default location (it is about a very specific location).

Having a `--save` flag that does the opposite of what the same flag does in `npm install` would be mega-trolling.
Turbo87: > Having a --save flag that does the opposite of what the same flag does in npm install would be mega-trolling.

sure, that's why I suggested to **not** use the word `save` at all here 😉 
xcambar: 👍 for considering that `--save` is indeed aligned with npm semantics, which makes it an easily remembered option.

@Turbo87 my grip with `--no-dev` though is the *negative* option. Here, "don't save to devDeps" does not read as "save to deps" to me.
stefanpenner: Although not perfect `--save` does seem alright.

@xcambar for future such submissions it may be good to also open an RFC, that way we can discuss the API design. I don't know if that makes sense any longer, but for future..


cc @ef4 would love your thoughts.
xcambar: @stefanpenner, I will, sorry for the disturbance in the Force 😁

ef4: This looks good to me.
stefanpenner: > @stefanpenner, I will, sorry for the disturbance in the Force 😁

No not a problem, we are still trying to find the right balance.
nathanhammond: Let's go all the way here:
* `ember install --save` **===** `npm install --save`
* `ember install --save-dev` **===** `npm install --save-dev`
* `ember install` **==** `npm install --save-dev` _(for backwards compat reasons)_

xcambar: Your comments have been applied, @nathanhammond.

Some notes:
* The aliases now reflect those of npm (with capital `S` and `D`, as per https://docs.npmjs.com/cli/install)
* Opposite to npm, in ember-cli, `--save` has a greater priority than `--save-dev` (_ie_, using `ember install my-addon -D -S` will save the package in `dependencies`).


nathanhammond: > Opposite to npm, in ember-cli, `--save` has a greater priority than `--save-dev` (_ie_, using `ember install my-addon -D -S` will save the package in `dependencies`).

If somebody tries to do both I'd rather we throw an error message than succeed at what is (to a user unfamiliar with the implementation) effectively `undefined` behavior. (Thank you for your patience as we work toward the ideal solution here.)
nathanhammond: @trabus I'm hoping that you can comment on https://github.com/ember-cli/ember-cli/pull/6017#discussion_r69246157 before we land this. I'm trying to avoid learning that portion of the code this week. 😜
nathanhammond: (@xcambar in spite of the back and forth on this, I'm quite excited to land this. Thank you for your patience and efforts thus far!)
trabus: @nathanhammond @xcambar Sorry for the delay, [see my reply here](https://github.com/ember-cli/ember-cli/pull/6017#discussion_r69669915)
xcambar: I reverted the `blueprintOptions`.

But we have a situation about options parsing: if `--save-dev` is true by default, then we have no way to turn it off. 
One possible way would be to have it false by default, then manually turn it to true in the task (if it applies, aka by default), but then the docs will indicate that `--save-dev` is false by default.

__tl;dr;__ we have to choose between:
* inconsistent inline documentation
* `--save-dev` can not be turned off
xcambar: I just realised I should have fixup'd the commit for better readability. Will do next time.
nathanhammond: > `--save-dev` can not be turned off

In order for an Ember CLI addon to work it must actually be present inside of `package.json` in the host application. We don't discover and run addons which are simply `npm install`ed. As a result of that it doesn't seem unreasonable to be unable to turn off `--save-dev` as otherwise the equivalent of `ember install foo --dont-write-to-package-json` is basically a no-op.
nathanhammond: @xcambar Looks good. With my last inline comment I'm ready to merge.

Patch:
diff --git a/lib/commands/install.js b/lib/commands/install.js
index bf123e850..0cf3bd8c6 100644
--- a/lib/commands/install.js
+++ b/lib/commands/install.js
@@ -10,6 +10,11 @@ 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/tasks/addon-install.js b/lib/tasks/addon-install.js
index f3da9adbd..4649916b7 100644
--- a/lib/tasks/addon-install.js
+++ b/lib/tasks/addon-install.js
@@ -16,8 +16,9 @@ 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,
@@ -35,8 +36,9 @@ module.exports = Task.extend({
     ui.startProgress(chalk.green('Installing addon package'), chalk.green('.'));
 
     return npmInstall.run({
-      packages: packageNames,
-      'save-dev': true,
+      'packages': packageNames,
+      'save': commandOptions.save,
+      'save-dev': !commandOptions.save && commandOptions.saveDev,
       'save-exact': true
     }).then(function() {
       return this.project.reloadAddons();
diff --git a/lib/tasks/npm-task.js b/lib/tasks/npm-task.js
index 3c5354b6d..e997d8a84 100644
--- a/lib/tasks/npm-task.js
+++ b/lib/tasks/npm-task.js
@@ -28,6 +28,7 @@ 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/fixtures/help/help-with-addon.txt b/tests/fixtures/help/help-with-addon.txt
index 15c0ad5ba..b3583fb5e 100644
--- a/tests/fixtures/help/help-with-addon.txt
+++ b/tests/fixtures/help/help-with-addon.txt
@@ -90,9 +90,13 @@ 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
+ember install \u001b[33m<addon-name>\u001b[39m \u001b[36m<options...>\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 128f78073..64ee93749 100644
--- a/tests/fixtures/help/help.js
+++ b/tests/fixtures/help/help.js
@@ -386,7 +386,22 @@ module.exports = {
       description: 'Installs an ember-cli addon from npm.',
       aliases: ['i'],
       works: 'insideProject',
-      availableOptions: [],
+      availableOptions: [
+        {
+          name: 'save',
+          default: false,
+          aliases: ['S'],
+          key: 'save',
+          required: false
+        },
+        {
+          name: 'save-dev',
+          default: true,
+          aliases: ['D'],
+          key: 'saveDev',
+          required: false
+        }
+      ],
       anonymousOptions: ['<addon-name>']
     },
     {
diff --git a/tests/fixtures/help/help.txt b/tests/fixtures/help/help.txt
index 743cd2a0c..c74e740bd 100644
--- a/tests/fixtures/help/help.txt
+++ b/tests/fixtures/help/help.txt
@@ -90,9 +90,13 @@ 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
+ember install \u001b[33m<addon-name>\u001b[39m \u001b[36m<options...>\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 322feb362..c7660abb1 100644
--- a/tests/fixtures/help/with-addon-blueprints.js
+++ b/tests/fixtures/help/with-addon-blueprints.js
@@ -418,7 +418,22 @@ module.exports = {
       description: 'Installs an ember-cli addon from npm.',
       aliases: ['i'],
       works: 'insideProject',
-      availableOptions: [],
+      availableOptions: [
+        {
+          name: 'save',
+          default: false,
+          aliases: ['S'],
+          key: 'save',
+          required: false
+        },
+        {
+          name: 'save-dev',
+          default: true,
+          aliases: ['D'],
+          key: 'saveDev',
+          required: false
+        }
+      ],
       anonymousOptions: ['<addon-name>']
     },
     {
diff --git a/tests/fixtures/help/with-addon-commands.js b/tests/fixtures/help/with-addon-commands.js
index eddc55ae0..82be7e949 100644
--- a/tests/fixtures/help/with-addon-commands.js
+++ b/tests/fixtures/help/with-addon-commands.js
@@ -386,7 +386,22 @@ module.exports = {
       description: 'Installs an ember-cli addon from npm.',
       aliases: ['i'],
       works: 'insideProject',
-      availableOptions: [],
+      availableOptions: [
+        {
+          name: 'save',
+          default: false,
+          aliases: ['S'],
+          key: 'save',
+          required: false
+        },
+        {
+          name: 'save-dev',
+          default: true,
+          aliases: ['D'],
+          key: 'saveDev',
+          required: false
+        }
+      ],
       anonymousOptions: ['<addon-name>']
     },
     {
diff --git a/tests/unit/commands/install-test.js b/tests/unit/commands/install-test.js
index a558d8425..15b74267d 100644
--- a/tests/unit/commands/install-test.js
+++ b/tests/unit/commands/install-test.js
@@ -99,12 +99,64 @@ 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;
@@ -143,6 +195,7 @@ 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});
@@ -159,6 +212,7 @@ describe('install command', function() {
 
         td.verify(npmRun({
           packages: ['ember-cli/ember-cli-qunit'],
+          'save': false,
           'save-dev': true,
           'save-exact': true
         }), {times: 1});
@@ -174,6 +228,7 @@ describe('install command', function() {
 
         td.verify(npmRun({
           packages: ['ember-cli-qunit@1.2.0'],
+          'save': false,
           'save-dev': true,
           'save-exact': true
         }), {times: 1});
@@ -190,6 +245,7 @@ describe('install command', function() {
 
         td.verify(npmRun({
           packages: ['@ember-cli/ember-cli-qunit'],
+          'save': false,
           'save-dev': true,
           'save-exact': true
         }), {times: 1});