Repository: ember-cli/ember-cli
Pull request #6515 (2016-12-02 22:17:54)

Avoid double walking of addon/templates in default scenario.
By default `addon-templates` path is `addon/templates/` and would have already been walked when we check `addon/`. This is just wasteful work that is done on first build for each addon.

rwjblue: > Also, how easy is this to test?

Unsure ATM, its mostly just reducing duplicated work so it isn't _wrong_ per-se (just wasteful of I/O when using the default `addon-templates` path).  

I think the longer term solution to testing this would be to setup a heimdalljs-fs-monitor testing harness, simulate the specific scenario, and ensure it avoids the extra `readdir` / `stat` / etc calls.

rwjblue: Updated to fix issue mentioned RE: `../foo` paths.  

I'd like to land and continue iterating on a better testing harness for this stuff.  I'm currently working on creating a harness to test addon broccoli methods more like you would test a broccoli-plugin, but am trying to split up unrelated refactors (so that I don't end up with a giant impossible to reason about PR)...

Patch:
diff --git a/blueprints/addon/files/addon-config/ember-try.js b/blueprints/addon/files/addon-config/ember-try.js
index df2613e30..b1813d0d7 100644
--- a/blueprints/addon/files/addon-config/ember-try.js
+++ b/blueprints/addon/files/addon-config/ember-try.js
@@ -5,8 +5,7 @@ module.exports = {
       name: 'ember-lts-2.4',
       bower: {
         dependencies: {
-          'ember': 'components/ember#lts-2-4',
-          'ember-cli-shims': '0.1.3'
+          'ember': 'components/ember#lts-2-4'
         },
         resolutions: {
           'ember': 'lts-2-4'
@@ -22,8 +21,7 @@ module.exports = {
       name: 'ember-lts-2.8',
       bower: {
         dependencies: {
-          'ember': 'components/ember#lts-2-8',
-          'ember-cli-shims': '0.1.3'
+          'ember': 'components/ember#lts-2-8'
         },
         resolutions: {
           'ember': 'lts-2-8'
@@ -39,8 +37,7 @@ module.exports = {
       name: 'ember-release',
       bower: {
         dependencies: {
-          'ember': 'components/ember#release',
-          'ember-cli-shims': '0.1.3'
+          'ember': 'components/ember#release'
         },
         resolutions: {
           'ember': 'release'
@@ -56,8 +53,7 @@ module.exports = {
       name: 'ember-beta',
       bower: {
         dependencies: {
-          'ember': 'components/ember#beta',
-          'ember-cli-shims': '0.1.3'
+          'ember': 'components/ember#beta'
         },
         resolutions: {
           'ember': 'beta'
@@ -73,8 +69,7 @@ module.exports = {
       name: 'ember-canary',
       bower: {
         dependencies: {
-          'ember': 'components/ember#canary',
-          'ember-cli-shims': '0.1.3'
+          'ember': 'components/ember#canary'
         },
         resolutions: {
           'ember': 'canary'
diff --git a/lib/broccoli/ember-app.js b/lib/broccoli/ember-app.js
index be8afc988..0abc94ce0 100644
--- a/lib/broccoli/ember-app.js
+++ b/lib/broccoli/ember-app.js
@@ -254,9 +254,7 @@ EmberApp.prototype._initOptions = function(options) {
 
   // For now we must disable Babel sourcemaps due to unforseen
   // performance regressions.
-  if (!('sourceMaps' in this.options.babel)) {
-    this.options.babel.sourceMaps = false;
-  }
+  this.options.babel.sourceMaps = false;
 };
 
 /**
diff --git a/lib/models/addon.js b/lib/models/addon.js
index ea11d9dd4..0a9e45f20 100644
--- a/lib/models/addon.js
+++ b/lib/models/addon.js
@@ -293,6 +293,10 @@ var Addon = CoreObject.extend({
     return tree;
   },
 
+  _treePathFor: function _treePathFor(treeName) {
+    return path.normalize(path.join(this.root, this.treePaths[treeName]));
+  },
+
   /* @private
    * @method _warn
    */
@@ -629,18 +633,17 @@ var Addon = CoreObject.extend({
   */
   shouldCompileTemplates: function() {
     var templateExtensions = this.registry.extensionsForType('template');
-    var addonTreePath = path.join(this.root, this.treePaths['addon']);
-    var addonTemplatesTreePath = path.join(this.root, this.treePaths['addon-templates']);
+    var addonTreePath = this._treePathFor('addon');
+    var addonTemplatesTreePath = this._treePathFor('addon-templates');
+    var addonTemplatesTreeInAddonTree = addonTemplatesTreePath.indexOf(addonTreePath) === 0;
 
-    var files = [];
+    var files = this._getAddonTreeFiles();
 
-    if (existsSync(addonTreePath)) {
-      files = files.concat(walkSync(addonTreePath));
+    if (!addonTemplatesTreeInAddonTree) {
+      files = files.concat(this._getAddonTemplatesTreeFiles());
     }
 
-    if (existsSync(addonTemplatesTreePath)) {
-      files = files.concat(walkSync(addonTemplatesTreePath));
-    }
+    files = files.filter(Boolean);
 
     var extensionMatcher = new RegExp('(' + templateExtensions.join('|') + ')$');
 
@@ -649,6 +652,26 @@ var Addon = CoreObject.extend({
     });
   },
 
+  _getAddonTreeFiles: function() {
+    var addonTreePath = this._treePathFor('addon');
+
+    if (existsSync(addonTreePath)) {
+      return walkSync(addonTreePath);
+    }
+
+    return [];
+  },
+
+  _getAddonTemplatesTreeFiles: function() {
+    var addonTemplatesTreePath = this._treePathFor('addon-templates');
+
+    if (existsSync(addonTemplatesTreePath)) {
+      return walkSync(addonTemplatesTreePath);
+    }
+
+    return [];
+  },
+
   _addonTemplateFiles: function addonTemplateFiles(tree) {
     this._requireBuildPackages();
 
@@ -750,7 +773,7 @@ var Addon = CoreObject.extend({
   jshintAddonTree: function() {
     this._requireBuildPackages();
 
-    var addonPath = path.join(this.root, this.treePaths['addon']);
+    var addonPath = this._treePathFor('addon');
 
     if (!existsSync(addonPath)) {
       return;


Test patch:
diff --git a/blueprints/app/files/tests/helpers/start-app.js b/blueprints/app/files/tests/helpers/start-app.js
index 9a605eb8d..e098f1d5b 100644
--- a/blueprints/app/files/tests/helpers/start-app.js
+++ b/blueprints/app/files/tests/helpers/start-app.js
@@ -3,13 +3,16 @@ import Application from '../../app';
 import config from '../../config/environment';
 
 export default function startApp(attrs) {
+  let application;
+
   let attributes = Ember.merge({}, config.APP);
   attributes = Ember.merge(attributes, attrs); // use defaults, but you can override;
 
-  return Ember.run(() => {
-    let application = Application.create(attributes);
+  Ember.run(() => {
+    application = Application.create(attributes);
     application.setupForTesting();
     application.injectTestHelpers();
-    return application;
   });
+
+  return application;
 }
diff --git a/tests/unit/broccoli/ember-app-test.js b/tests/unit/broccoli/ember-app-test.js
index 8b3edf76f..f1bbb6201 100644
--- a/tests/unit/broccoli/ember-app-test.js
+++ b/tests/unit/broccoli/ember-app-test.js
@@ -209,27 +209,6 @@ describe('broccoli/ember-app', function() {
         expect(app.vendorFiles['ember-resolver']).to.equal(undefined);
       });
     });
-
-    describe('options.babel.sourceMaps', function() {
-      it('disables babel sourcemaps by default', function() {
-        var app = new EmberApp({
-          project: project
-        });
-
-        expect(app.options.babel.sourceMaps).to.be.false;
-      });
-
-      it('can enable babel sourcemaps with the option', function() {
-        var app = new EmberApp({
-          project: project,
-          babel: {
-            sourceMaps: 'inline'
-          }
-        });
-
-        expect(app.options.babel.sourceMaps).to.equal('inline');
-      });
-    });
   });
 
   describe('contentFor', function() {
diff --git a/tests/unit/models/addon-test.js b/tests/unit/models/addon-test.js
index 9d0a90908..658ba0ec5 100644
--- a/tests/unit/models/addon-test.js
+++ b/tests/unit/models/addon-test.js
@@ -619,6 +619,33 @@ describe('models/addon.js', function() {
         addon.compileTemplates();
       }).not.to.throw();
     });
+
+    it('should not call _getAddonTemplatesTreeFiles when default treePath is used', function() {
+      addon.root = path.join(fixturePath, 'with-empty-addon-templates');
+      var wasCalled = false;
+      addon._getAddonTemplatesTreeFiles = function() {
+        wasCalled = true;
+        return [];
+      };
+
+      addon.compileTemplates();
+
+      expect(wasCalled).to.not.be.ok;
+    });
+
+    it('should call _getAddonTemplatesTreeFiles when custom treePaths[\'addon-templates\'] is used', function() {
+      addon.root = path.join(fixturePath, 'with-empty-addon-templates');
+      addon.treePaths['addon-templates'] = 'foo';
+      var wasCalled = false;
+      addon._getAddonTemplatesTreeFiles = function() {
+        wasCalled = true;
+        return [];
+      };
+
+      addon.compileTemplates();
+
+      expect(wasCalled).to.be.ok;
+    });
   });
 
   describe('addonDiscovery', function() {