Repository: ember-cli/ember-cli
Pull request #6241 (2016-09-08 18:14:13)

destroy deletes empty folders
Wanted to get some feedback while I finish the tests.

This has been reported as a bug a few times before, so here's the fix. When a blueprint destroy removes a file, it checks if the dir is now empty and deletes. Then recurses up the tree and stops at `app`.

I see this as a bugfix and not something that needs to go through RFC. Thoughts?

stefanpenner: I don't believe we should do this. Folder emptiness does not indicate we can safely remove it. 
kellyselden: @stefanpenner Do you know of an example when this is bad?
nathanhammond: At least recently (don't know if this is still the case) deleting folders out from underneath Broccoli would result in an error. @stefanpenner and @hjdivad can confirm if they believe this is fixed.

Other bad scenarios have to do with addons relying on a certain folder existing and doing `new Funnel('/boom')`.
kellyselden: @stefanpenner @nathanhammond Is there any compromise on this? Should it go to RFC? Personally I find it annoying whenever I destroy a route or whatever that I have to clean up the empty folders, just from an OCD/cleanliness perspective. It makes the `destroy` command "incomplete" for my goal of getting the state back to before the `generate`.

On the other hand, I do realize the risk of deleting something the user wasn't expecting.
hjdivad: > At least recently (don't know if this is still the case) deleting folders out from underneath Broccoli would result in an error. @stefanpenner and @hjdivad can confirm if they believe this is fixed.

That should be fixed in `fs-tree-diff@^0.5.0` IIRC
stefanpenner: > At least recently (don't know if this is still the case) deleting folders out from underneath Broccoli would result in an error. @stefanpenner and @hjdivad can confirm if they believe this is fixed.

ya we fixed this.

If the general consensus here is, blueprint destroy should destroy empty folders then we can move forward. 
nathanhammond: This is on the agenda for Thursday. :smile:
hjdivad: > If the general consensus here is, blueprint destroy should destroy empty folders then we can move forward.

I agree with @kellyselden on this.  It seems weird for generate/destroy to not leave you back in the state you started from.  I think @kellyselden's suggestion will make the common cases less surprising for people.

Removing all empty dirs up to `app` will have some pathological edge cases, but `.gitkeep` and equivalents seem like a reasonable way to deal with this.
nathanhammond: - [ ] Add test for checking symlink behavior.
kellyselden: I ended up adding an additional property to `FileInfo` called `outputBasePath` to help in assembling new paths when traversing up.
kellyselden: I finished my testing. The CLI doesn't let you put in root paths or '../..', so I think we are covered there. I wrote a symlink test, and it worked just like a normal file preserving it. It didn't seem all that useful a test to me. Thoughts @nathanhammond @stefanpenner ? Maybe I'm missing the scenario where a symlink test would be good?

Other than that, this is ready to go.
nathanhammond: Haven't reviewed `cleanRemove` ... will get to it after your next update.
nathanhammond: | --- | --- |
| thing | thing2 |
| --- | --- |

nathanhammond: @homu r+
homu: :pushpin: Commit 6a53154 has been approved by `nathanhammond`

<!-- @homu r=nathanhammond 6a53154ac28064e14b3284508f25864e40045a1d -->
homu: :hourglass: Testing commit 6a53154 with merge 1cae632...

Patch:
diff --git a/lib/models/blueprint.js b/lib/models/blueprint.js
index c1fc3c5aa..190804359 100644
--- a/lib/models/blueprint.js
+++ b/lib/models/blueprint.js
@@ -11,6 +11,7 @@ var printableProperties = require('../utilities/printable-properties').blueprint
 var sequence            = require('../utilities/sequence');
 var printCommand        = require('../utilities/print-command');
 var insertIntoFile      = require('../utilities/insert-into-file');
+var cleanRemove         = require('../utilities/clean-remove');
 var fs                  = require('fs-extra');
 var existsSync          = require('exists-sync');
 var inflector           = require('inflection');
@@ -28,7 +29,6 @@ var normalizeEntityName = require('ember-cli-normalize-entity-name');
 
 var stat                = Promise.denodeify(fs.stat);
 var writeFile           = Promise.denodeify(fs.outputFile);
-var removeFile          = Promise.denodeify(fs.remove);
 
 /**
   A blueprint is a bundle of template files with optional install
@@ -328,7 +328,7 @@ var Blueprint = CoreObject.extend({
     remove: function(info) {
       this._writeStatusToUI(chalk.red, 'remove', info.displayPath);
       if (!this.dryRun) {
-        return removeFile(info.outputPath);
+        return cleanRemove(info);
       }
     }
   },
@@ -639,12 +639,13 @@ var Blueprint = CoreObject.extend({
     @param {String} file
     @return {FileInfo}
   */
-  buildFileInfo: function(destPath, templateVariables, file) {
+  buildFileInfo: function(intoDir, templateVariables, file) {
     var mappedPath = this.mapFile(file, templateVariables);
 
     return new FileInfo({
       action: 'write',
-      outputPath: destPath(mappedPath),
+      outputBasePath: path.normalize(intoDir),
+      outputPath: path.join(intoDir, mappedPath),
       displayPath: path.normalize(mappedPath),
       inputPath: this.srcPath(file),
       templateVariables: templateVariables,
@@ -671,7 +672,7 @@ var Blueprint = CoreObject.extend({
     @return {Array} file infos
   */
   _getFileInfos: function(files, intoDir, templateVariables) {
-    return files.map(this.buildFileInfo.bind(this, destPath.bind(null, intoDir), templateVariables));
+    return files.map(this.buildFileInfo.bind(this, intoDir, templateVariables));
   },
 
   /**
@@ -1480,17 +1481,6 @@ function podDeprecations(config, ui) {
   }
 }
 
-/**
-  @private
-  @method destPath
-  @param {String} intoDir
-  @param {String} file
-  @return {String} new path
-*/
-function destPath(intoDir, file) {
-  return path.join(intoDir, file);
-}
-
 /**
   @private
   @method isValidFile
diff --git a/lib/models/file-info.js b/lib/models/file-info.js
index 7cd47657c..fd38116f2 100644
--- a/lib/models/file-info.js
+++ b/lib/models/file-info.js
@@ -76,6 +76,7 @@ FileInfo.prototype.displayDiff = function() {
 
 function FileInfo(options) {
   this.action = options.action;
+  this.outputBasePath = options.outputBasePath;
   this.outputPath = options.outputPath;
   this.displayPath = options.displayPath;
   this.inputPath =  options.inputPath;
diff --git a/lib/utilities/clean-remove.js b/lib/utilities/clean-remove.js
new file mode 100644
index 000000000..02bec1ee4
--- /dev/null
+++ b/lib/utilities/clean-remove.js
@@ -0,0 +1,52 @@
+'use strict';
+
+var path = require('path');
+var fs = require('fs-extra');
+var Promise = require('../ext/promise');
+var walkUp = require('./walk-up-path');
+var stat = Promise.denodeify(fs.stat);
+var remove = Promise.denodeify(fs.remove);
+var readdir = Promise.denodeify(fs.readdir);
+
+function cleanRemove(fileInfo) {
+  // see if file exists to avoid wasting time
+  return stat(fileInfo.outputPath).then(function() {
+    return remove(fileInfo.outputPath).then(function() {
+      var paths = walkUp(fileInfo.displayPath).map(function(thePath) {
+        return path.join(fileInfo.outputBasePath, thePath);
+      });
+
+      return paths.reduce(function(chainedPromise, thePath) {
+        return chainedPromise.then(function(wasShortCircuited) {
+          if (wasShortCircuited) {
+            // optimization that says since my child dir wasn't empty,
+            // I can't be empty, so keep skipping
+            return true;
+          }
+
+          // get list of files remaining in this dir
+          return readdir(thePath).then(function(paths) {
+            if (paths.length) {
+              // don't check parent dirs since this one isn't empty
+              return true;
+            }
+
+            return remove(thePath).then(function() {
+              return false;
+            });
+          });
+        });
+      }, Promise.resolve());
+    });
+  }).catch(function(err) {
+    if (err.code !== 'ENOENT') {
+      throw err;
+    }
+
+    // you tried to destroy a blueprint without first generating it
+    // instead of trying to read dirs that don't exist
+    // swallow error and carry on
+  });
+}
+
+module.exports = cleanRemove;
diff --git a/lib/utilities/walk-up-path.js b/lib/utilities/walk-up-path.js
new file mode 100644
index 000000000..cbe03bd1f
--- /dev/null
+++ b/lib/utilities/walk-up-path.js
@@ -0,0 +1,22 @@
+'use strict';
+
+var path = require('path');
+
+var regex = /^[\./]$/;
+
+function walkUp(thePath) {
+  var paths = [];
+
+  var currentPath = thePath;
+  while (true) { // eslint-disable-line no-constant-condition
+    currentPath = path.dirname(currentPath);
+    if (regex.test(currentPath)) {
+      break;
+    }
+    paths.push(currentPath);
+  }
+
+  return paths;
+}
+
+module.exports = walkUp;


Test patch:
diff --git a/tests/fixtures/blueprints/basic/files/__root__/__path__/__name__.txt b/tests/fixtures/blueprints/basic/files/__root__/__path__/__name__.txt
new file mode 100644
index 000000000..02f6335fc
--- /dev/null
+++ b/tests/fixtures/blueprints/basic/files/__root__/__path__/__name__.txt
@@ -0,0 +1,1 @@
+a file
diff --git a/tests/integration/utilities/clean-remove-test.js b/tests/integration/utilities/clean-remove-test.js
new file mode 100644
index 000000000..a272cb2d7
--- /dev/null
+++ b/tests/integration/utilities/clean-remove-test.js
@@ -0,0 +1,79 @@
+'use strict';
+
+var expect = require('chai').expect;
+var cleanRemove = require('../../../lib/utilities/clean-remove');
+var temp = require('temp');
+var path = require('path');
+var Promise = require('../../../lib/ext/promise');
+var fs = require('fs-extra');
+
+var outputFile = Promise.denodeify(fs.outputFile);
+var stat = Promise.denodeify(fs.stat);
+
+describe('clean-remove', function() {
+  var tempDir;
+  var originalCwd = process.cwd();
+  var fileInfo;
+  var nestedPath = 'nested1/nested2';
+
+  beforeEach(function() {
+    tempDir = temp.mkdirSync('clean-remove');
+    process.chdir(tempDir);
+
+    fileInfo = {
+      outputBasePath: tempDir
+    };
+  });
+
+  afterEach(function() {
+    process.chdir(originalCwd);
+    fs.removeSync(tempDir);
+  });
+
+  it('removes empty folders', function() {
+    var displayPath = path.join(nestedPath, 'file.txt');
+    fileInfo.outputPath = path.join(tempDir, displayPath);
+    fileInfo.displayPath = displayPath;
+
+    return outputFile(displayPath, '').then(function() {
+      return stat(displayPath).then(function(stats) {
+        expect(stats).to.be.ok;
+      });
+    }).then(function() {
+      return cleanRemove(fileInfo);
+    }).then(function() {
+      return stat('nested1').then(function() {
+        expect(false).to.be.ok;
+      }).catch(function(err) {
+        expect(err).to.be.ok;
+      });
+    });
+  });
+
+  it('preserves filled folders', function() {
+    var removedDisplayPath = path.join(nestedPath, 'file.txt');
+    var preservedDisplayPath = path.join(nestedPath, 'file2.txt');
+    fileInfo.outputPath = path.join(tempDir, removedDisplayPath);
+    fileInfo.displayPath = removedDisplayPath;
+
+    return outputFile(removedDisplayPath, '').then(function() {
+      return outputFile(preservedDisplayPath, '');
+    }).then(function() {
+      return stat(preservedDisplayPath).then(function(stats) {
+        expect(stats).to.be.ok;
+      });
+    }).then(function() {
+      return cleanRemove(fileInfo);
+    }).then(function() {
+      return stat(removedDisplayPath).then(function() {
+        expect(false).to.be.ok;
+      }).catch(function(err) {
+        expect(err).to.be.ok;
+      });
+    }).then(function() {
+      return stat(preservedDisplayPath).then(function(stats) {
+        expect(stats).to.be.ok;
+      });
+    });
+  });
+});
diff --git a/tests/unit/models/blueprint-test.js b/tests/unit/models/blueprint-test.js
index cd7cc93b0..8d38d5852 100644
--- a/tests/unit/models/blueprint-test.js
+++ b/tests/unit/models/blueprint-test.js
@@ -32,6 +32,17 @@ var beforeInstallCalled;
 var afterInstallCalled;
 var afterUninstallCalled;
 
+function resetCalled() {
+  localsCalled = false;
+  normalizeEntityNameCalled = false;
+  fileMapTokensCalled = false;
+  filesPathCalled = false;
+  beforeUninstallCalled = false;
+  beforeInstallCalled = false;
+  afterInstallCalled = false;
+  afterUninstallCalled = false;
+}
+
 var instrumented = {
   locals: function(opts) {
     localsCalled = true;
@@ -109,6 +120,9 @@ var defaultIgnoredFiles = Blueprint.ignoredFiles;
 var basicBlueprintFiles = [
   '.ember-cli',
   '.gitignore',
+  'app/',
+  'app/basics/',
+  'app/basics/mock-project.txt',
   'bar',
   'foo.txt',
   'test.txt'
@@ -121,14 +135,7 @@ describe('Blueprint', function() {
   beforeEach(function() {
     Blueprint.ignoredFiles = defaultIgnoredFiles;
 
-    localsCalled = false;
-    normalizeEntityNameCalled = false;
-    fileMapTokensCalled = false;
-    filesPathCalled = false;
-    beforeUninstallCalled = false;
-    beforeInstallCalled = false;
-    afterInstallCalled = false;
-    afterUninstallCalled = false;
+    resetCalled();
 
     existsSyncStub = existsSync;
     readdirSyncStub = fs.readdirSync;
@@ -552,6 +559,7 @@ help in detail');
         expect(output.shift()).to.match(/^installing/);
         expect(output.shift()).to.match(/create.* .ember-cli/);
         expect(output.shift()).to.match(/create.* .gitignore/);
+        expect(output.shift()).to.match(/create.* app[/\\]basics[/\\]mock-project.txt/);
         expect(output.shift()).to.match(/create.* bar/);
         expect(output.shift()).to.match(/create.* foo.txt/);
         expect(output.shift()).to.match(/create.* test.txt/);
@@ -580,6 +588,7 @@ help in detail');
         expect(output.shift()).to.match(/^installing/);
         expect(output.shift()).to.match(/create.* .ember-cli/);
         expect(output.shift()).to.match(/create.* .gitignore/);
+        expect(output.shift()).to.match(/create.* app[/\\]basics[/\\]mock-project.txt/);
         expect(output.shift()).to.match(/create.* bar/);
         expect(output.shift()).to.match(/create.* foo.txt/);
         expect(output.shift()).to.match(/create.* test.txt/);
@@ -594,6 +603,7 @@ help in detail');
         expect(output.shift()).to.match(/^installing/);
         expect(output.shift()).to.match(/identical.* .ember-cli/);
         expect(output.shift()).to.match(/identical.* .gitignore/);
+        expect(output.shift()).to.match(/identical.* app[/\\]basics[/\\]mock-project.txt/);
         expect(output.shift()).to.match(/identical.* bar/);
         expect(output.shift()).to.match(/identical.* foo.txt/);
         expect(output.shift()).to.match(/identical.* test.txt/);
@@ -616,6 +626,7 @@ help in detail');
         expect(output.shift()).to.match(/^installing/);
         expect(output.shift()).to.match(/create.* .ember-cli/);
         expect(output.shift()).to.match(/create.* .gitignore/);
+        expect(output.shift()).to.match(/create.* app[/\\]basics[/\\]mock-project.txt/);
         expect(output.shift()).to.match(/create.* bar/);
         expect(output.shift()).to.match(/create.* foo.txt/);
         expect(output.shift()).to.match(/create.* test.txt/);
@@ -702,6 +713,7 @@ help in detail');
           expect(output.shift()).to.match(/^installing/);
           expect(output.shift()).to.match(/create.* .ember-cli/);
           expect(output.shift()).to.match(/create.* .gitignore/);
+          expect(output.shift()).to.match(/create.* app[/\\]basics[/\\]mock-project.txt/);
           expect(output.shift()).to.match(/create.* bar/);
           expect(output.shift()).to.match(/create.* foo.txt/);
           expect(output.shift()).to.match(/create.* test.txt/);
@@ -778,7 +790,8 @@ help in detail');
       .then(function() {
         var actualFiles = walkSync(tmpdir).sort();
 
-        expect(actualFiles).to.deep.equal(basicBlueprintFiles);
+        expect(actualFiles).to.contain('app/basics/foo.txt');
+        expect(actualFiles).to.not.contain('app/basics/mock-project.txt');
       });
     });
 
@@ -854,6 +867,7 @@ help in detail');
         expect(output.shift()).to.match(/^uninstalling/);
         expect(output.shift()).to.match(/remove.* .ember-cli/);
         expect(output.shift()).to.match(/remove.* .gitignore/);
+        expect(output.shift()).to.match(/remove.* app[/\\]basics[/\\]mock-project.txt/);
         expect(output.shift()).to.match(/remove.* bar/);
         expect(output.shift()).to.match(/remove.* foo.txt/);
         expect(output.shift()).to.match(/remove.* test.txt/);
@@ -867,47 +881,65 @@ help in detail');
       });
     });
 
-    describe('instrumented blueprint uninstallation', function() {
-      var blueprint;
-      var ui;
-      var project;
-      var options;
-      var tmpdir;
+    it('uninstall doesn\'t remove non-empty folders', function() {
+      options.entity = { name: 'foo' };
 
-      function refreshUI() {
-        ui = new MockUI();
-        options.ui = ui;
-      }
+      return blueprint.install(options).then(function() {
+        var actualFiles = walkSync(tmpdir);
 
-      beforeEach(function() {
-        return mkTmpDirIn(tmproot).then(function(dir) {
-          tmpdir = dir;
-          blueprint = new InstrumentedBasicBlueprint(basicBlueprint);
-          project   = new MockProject();
-          options   = {
-            project: project,
-            target: tmpdir
-          };
-          refreshUI();
-
-          return blueprint.uninstall(options);
-        }).then(refreshUI);
-      });
-
-      it('calls appropriate hooks with correct arguments', function() {
-        options.entity = { name: 'foo' };
-
-        return blueprint.uninstall(options).then(function() {
-          expect(localsCalled).to.be.true;
-          expect(normalizeEntityNameCalled).to.be.true;
-          expect(fileMapTokensCalled).to.be.true;
-          expect(filesPathCalled).to.be.true;
-          expect(beforeUninstallCalled).to.be.true;
-          expect(afterUninstallCalled).to.be.true;
-
-          expect(beforeInstallCalled).to.be.false;
-          expect(afterInstallCalled).to.be.false;
-        });
+        expect(actualFiles).to.contain('app/basics/foo.txt');
+        expect(actualFiles).to.contain('app/basics/mock-project.txt');
+
+        return blueprint.uninstall(options);
+      }).then(function() {
+        var actualFiles = walkSync(tmpdir);
+
+        expect(actualFiles).to.not.contain('app/basics/foo.txt');
+        expect(actualFiles).to.contain('app/basics/mock-project.txt');
+      });
+    });
+  });
+
+  describe('instrumented blueprint uninstallation', function() {
+    var blueprint;
+    var ui;
+    var project;
+    var options;
+    var tmpdir;
+
+    function refreshUI() {
+      ui = new MockUI();
+      options.ui = ui;
+    }
+
+    beforeEach(function() {
+      return mkTmpDirIn(tmproot).then(function(dir) {
+        tmpdir = dir;
+        blueprint = new InstrumentedBasicBlueprint(basicBlueprint);
+        project   = new MockProject();
+        options   = {
+          project: project,
+          target: tmpdir
+        };
+        refreshUI();
+
+        return blueprint.install(options).then(resetCalled);
+      }).then(refreshUI);
+    });
+
+    it('calls appropriate hooks with correct arguments', function() {
+      options.entity = { name: 'foo' };
+
+      return blueprint.uninstall(options).then(function() {
+        expect(localsCalled).to.be.true;
+        expect(normalizeEntityNameCalled).to.be.true;
+        expect(fileMapTokensCalled).to.be.true;
+        expect(filesPathCalled).to.be.true;
+        expect(beforeUninstallCalled).to.be.true;
+        expect(afterUninstallCalled).to.be.true;
+
+        expect(beforeInstallCalled).to.be.false;
+        expect(afterInstallCalled).to.be.false;
       });
     });
   });
diff --git a/tests/unit/utilities/walk-up-path-test.js b/tests/unit/utilities/walk-up-path-test.js
new file mode 100644
index 000000000..3ec9ec945
--- /dev/null
+++ b/tests/unit/utilities/walk-up-path-test.js
@@ -0,0 +1,35 @@
+'use strict';
+
+var expect = require('chai').expect;
+var walkUp = require('../../../lib/utilities/walk-up-path');
+
+describe('walk-up-path', function() {
+  it('walks up paths', function() {
+    var paths = walkUp('app/templates/my/test.hbs');
+
+    expect(paths).to.deep.equal([
+      'app/templates/my',
+      'app/templates',
+      'app',
+    ]);
+  });
+
+  it('walks up paths beginning with a slash', function() {
+    var paths = walkUp('/app/templates/my/test.hbs');
+
+    expect(paths).to.deep.equal([
+      '/app/templates/my',
+      '/app/templates',
+      '/app',
+    ]);
+  });
+
+  it('walks up paths that have same folder names', function() {
+    var paths = walkUp('foo/foo/bar.hbs');
+
+    expect(paths).to.deep.equal([
+      'foo/foo',
+      'foo'
+    ]);
+  });
+});