Repository: ember-cli/ember-cli
Pull request #8143 (2018-10-18 01:51:41)

adding fix to use NULL nodeModuleList to optimize when dealing with non-addons
courtesy of spenner, with some adjustments of my own.

dcombslinkedin: Fails in AppVeyor have nothing to do with the package-info-cache code. They're things like missing directories or files in other tests.
stefanpenner: This lgtm. But I am just about to head on vacation so I’ll leave this to @rwjblue 
dcombslinkedin: Thanks, @stefanpenner. Have a good vacation!

Patch:
diff --git a/lib/models/package-info-cache/error-list.js b/lib/models/package-info-cache/error-list.js
index 507e78007..8ceb43ce5 100644
--- a/lib/models/package-info-cache/error-list.js
+++ b/lib/models/package-info-cache/error-list.js
@@ -10,7 +10,7 @@ const ErrorEntry = require('./error-entry');
  * @protected
  * @class ErrorList
  */
-class ErrorList {
+module.exports = class ErrorList {
   constructor() {
     this.errors = [];
   }
@@ -36,6 +36,4 @@ class ErrorList {
   hasErrors() {
     return this.errors.length > 0;
   }
-}
-
-module.exports = ErrorList;
+};
diff --git a/lib/models/package-info-cache/index.js b/lib/models/package-info-cache/index.js
index 62c9505cf..265dc33ba 100644
--- a/lib/models/package-info-cache/index.js
+++ b/lib/models/package-info-cache/index.js
@@ -247,7 +247,7 @@ class PackageInfoCache {
    */
   loadProject(projectInstance) {
 
-    let pkgInfo = this._readPackage(projectInstance.root, projectInstance.pkg);
+    let pkgInfo = this._readPackage(projectInstance.root, projectInstance.pkg, true);
 
     // NOTE: the returned val may contain errors, or may contain
     // other packages that have errors. We will try to process
@@ -501,8 +501,10 @@ class PackageInfoCache {
    * @method _readPackage
    * @param {String} pkgDir the path of the directory to read the package.json from and
    *  process the contents and create a new cache entry or entries.
+   * @param {Boolean} isRoot, for when this is to be considered the root
+   * package, whose dependencies we must all consider for discovery.
    */
-  _readPackage(packageDir, pkg) {
+  _readPackage(packageDir, pkg, isRoot) {
     let normalizedPackageDir = path.normalize(packageDir);
 
     // Most of the time, normalizedPackageDir is already a real path (i.e. fs.realpathSync
@@ -579,7 +581,7 @@ class PackageInfoCache {
 
     // Create a new PackageInfo and load any errors as needed.
     // Note that pkg may be an empty object here.
-    pkgInfo = new PackageInfo(pkg, realPath, this);
+    pkgInfo = new PackageInfo(pkg, realPath, this, isRoot);
 
     if (setupErrors.hasErrors()) {
       pkgInfo.errors = setupErrors;
@@ -588,8 +590,7 @@ class PackageInfoCache {
 
     // If we have an ember-addon, check that the main exists and points
     // to a valid file.
-    if (Array.isArray(pkg.keywords) &&
-        pkg.keywords.indexOf('ember-addon') >= 0) {
+    if (pkgInfo.isAddon()) {
 
       // Note: when we have both 'main' and ember-addon:main, the latter takes precedence
       let main = (pkg['ember-addon'] && pkg['ember-addon'].main) || pkg['main'];
@@ -633,15 +634,21 @@ class PackageInfoCache {
       }
     }
 
-    // read addon modules from node_modules. We read the whole directory
-    // because it's assumed that npm/yarn may have placed addons in the
-    // directory from lower down in the project tree, and we want to get
-    // the data into the cache ASAP. It may not necessarily be a 'real' error
-    // if we find an issue, if nobody below is actually invoking the addon.
-    let nodeModules = this._readNodeModulesList(path.join(realPath, 'node_modules'));
+    if (pkgInfo.mayHaveAddons) {
+      // read addon modules from node_modules. We read the whole directory
+      // because it's assumed that npm/yarn may have placed addons in the
+      // directory from lower down in the project tree, and we want to get
+      // the data into the cache ASAP. It may not necessarily be a 'real' error
+      // if we find an issue, if nobody below is actually invoking the addon.
+      let nodeModules = this._readNodeModulesList(path.join(realPath, 'node_modules'));
 
-    if (nodeModules) {
-      pkgInfo.nodeModules = nodeModules;
+      if (nodeModules) {
+        pkgInfo.nodeModules = nodeModules;
+      }
+    } else {
+      // will not have addons, so even if there are node_modules here, we can
+      // simply pretend there are none.
+      pkgInfo.nodeModules = NodeModulesList.NULL;
     }
 
     return pkgInfo;
diff --git a/lib/models/package-info-cache/node-modules-list.js b/lib/models/package-info-cache/node-modules-list.js
index 9a544c2e4..51b4ec153 100644
--- a/lib/models/package-info-cache/node-modules-list.js
+++ b/lib/models/package-info-cache/node-modules-list.js
@@ -2,6 +2,8 @@
 
 const ErrorList = require('./error-list');
 
+let NULL;
+
 /**
  * Class that stores information about a node_modules directory (i.e., the
  * packages and subdirectories in the directory). It is one of the
@@ -11,12 +13,31 @@ const ErrorList = require('./error-list');
  * @public
  * @class NodeModulesList
  */
-class NodeModulesList {
+module.exports = class NodeModulesList {
   constructor(realPath, cache) {
     this.realPath = realPath;
-    this.cache = cache;
+    this.hasEntries = false; // for speed
     this.entries = Object.create(null);
     this.errors = new ErrorList();
+    this.cache = cache;
+  }
+
+  // when we encounter a node_modules we will never traverse, we insert a NULL variant.
+  // https://en.wikipedia.org/wiki/Null_object_pattern
+  // returns a Frozen and Empty NodeModulesList
+  static get NULL() {
+    if (NULL) {
+      return NULL;
+    }
+
+    NULL = new this('/dev/null'); // could be anywhere, why not /dev/null?
+
+    Object.freeze(NULL.entries);
+    Object.freeze(NULL.errors.errors);
+    Object.freeze(NULL.errors);
+    Object.freeze(NULL);
+
+    return NULL;
   }
 
   /**
@@ -56,6 +77,7 @@ class NodeModulesList {
    * to the given entry name in the file system.
    */
   addEntry(entryName, entryVal) {
+    this.hasEntries = true;
     this.entries[entryName] = entryVal;
   }
 
@@ -70,6 +92,8 @@ class NodeModulesList {
    * @return the desired PackageInfo if one exists for the given name, else null.
    */
   findPackage(packageName) {
+    if (!this.hasEntries) { return null; } // for speed
+
     let val;
 
     if (packageName.startsWith('@')) {
@@ -80,11 +104,9 @@ class NodeModulesList {
           ? entry.findPackage(parts[1]) // the real name
           : null;
     } else {
-      val = this.entries[packageName];
+      val = (this.entries[packageName] || null);
     }
 
     return val;
   }
-}
-
-module.exports = NodeModulesList;
+};
diff --git a/lib/models/package-info-cache/package-info.js b/lib/models/package-info-cache/package-info.js
index fcfe68b1e..748e8dc86 100644
--- a/lib/models/package-info-cache/package-info.js
+++ b/lib/models/package-info-cache/package-info.js
@@ -5,6 +5,7 @@ const ErrorList = require('./error-list');
 const Errors = require('./errors');
 const AddonInfo = require('../addon-info');
 const isAddon = require('../../utilities/is-addon');
+const logger = require('heimdalljs-logger')('ember-cli:package-info-cache:package-info');
 
 function lexicographically(a, b) {
   const aIsString = typeof a.name === 'string';
@@ -53,7 +54,7 @@ function pushUnique(array, entry) {
  * @class PackageInfo
  */
 class PackageInfo {
-  constructor(pkgObj, realPath, cache) {
+  constructor(pkgObj, realPath, cache, isRoot = false) {
     this.pkg = pkgObj;
     this.pkg['ember-addon'] = this.pkg['ember-addon'] || {};
     this.realPath = realPath;
@@ -79,6 +80,8 @@ class PackageInfo {
     // not actually be used.
     this.valid = true;
 
+    this.mayHaveAddons = isRoot || this.isAddon(); // mayHaveAddons used in index.js
+
     this._hasDumpedInvalidAddonPackages = false;
   }
 
@@ -336,24 +339,22 @@ class PackageInfo {
 
     let invalidPackages = this.getInvalidPackages(addonPackageList);
 
-    let ui = this.cache.ui;
-
-    if (invalidPackages.length > 0 && ui && ui.writeWarnLine) {
+    if (invalidPackages.length > 0) {
       let typeName = (this.project ? 'project' : 'addon');
 
-      ui.writeWarnLine('');
-      ui.writeWarnLine(`The 'package.json' file for the ${typeName} at ${this.realPath}`);
+      logger.info('');
+      logger.info(`The 'package.json' file for the ${typeName} at ${this.realPath}`);
 
       let relativePath;
 
       if (invalidPackages.length === 1) {
         relativePath = path.relative(this.realPath, invalidPackages[0].realPath);
-        ui.writeWarnLine(`  specifies an invalid, malformed or missing addon at relative path '${relativePath}'`);
+        logger.info(`  specifies an invalid, malformed or missing addon at relative path '${relativePath}'`);
       } else {
-        ui.writeWarnLine('  specifies invalid, malformed or missing addons at relative paths');
+        logger.info('  specifies invalid, malformed or missing addons at relative paths');
         invalidPackages.forEach(packageInfo => {
           let relativePath = path.relative(this.realPath, packageInfo.realPath);
-          ui.writeWarnLine(`    '${relativePath}'`);
+          logger.info(`    '${relativePath}'`);
         });
       }
     }


Test patch:
diff --git a/tests/unit/models/package-info-cache/node-module-list-test.js b/tests/unit/models/package-info-cache/node-module-list-test.js
new file mode 100644
index 000000000..921d5c8cf
--- /dev/null
+++ b/tests/unit/models/package-info-cache/node-module-list-test.js
@@ -0,0 +1,45 @@
+'use strict';
+
+const expect = require('chai').expect;
+const NodeModulesList = require('../../../../lib/models/package-info-cache/node-modules-list');
+
+describe('models/package-info-cache/node-modules-list-test', function() {
+  it('correctly constructs', function() {
+    expect(new NodeModulesList()).to.be.ok;
+    expect(new NodeModulesList('/some/path')).to.be.ok;
+  });
+
+  describe('.NULL', function() {
+    it('returns a singleton, deeply frozen NodeMoudlesList', function() {
+      expect(NodeModulesList.NULL).to.equal(NodeModulesList.NULL);
+      expect(NodeModulesList.NULL).to.be.frozen;
+      expect(NodeModulesList.NULL.entries).to.be.frozen;
+      expect(NodeModulesList.NULL.errors).to.be.frozen;
+      expect(NodeModulesList.NULL.errors.errors).to.be.frozen;
+    });
+  });
+
+  describe('findPackage', function() {
+    it('works with no entries', function() {
+      let list = new NodeModulesList();
+      expect(list.findPackage('omg')).to.eql(null);
+    });
+
+    it('supports basic entries (missing, present, scoped)', function() {
+      let list = new NodeModulesList();
+      let scoped = new NodeModulesList();
+      let omg = { name: 'omg' };
+      let scopedOmg = { name: 'omg' };
+      scoped.addEntry('omg', scopedOmg);
+
+      list.addEntry('omg', omg);
+      list.addEntry('@thescope', scoped);
+
+      expect(list.findPackage('omg')).to.eql(omg);
+      expect(list.findPackage('nope')).to.eql(null);
+      expect(list.findPackage('@thescope/omg')).to.eql(scopedOmg);
+      expect(list.findPackage('@thescope/nope')).to.eql(null);
+      expect(list.findPackage('@nope/nope')).to.eql(null);
+    });
+  });
+});
diff --git a/tests/unit/models/package-info-cache-test.js b/tests/unit/models/package-info-cache/package-info-cache-test.js
similarity index 93%
rename from tests/unit/models/package-info-cache-test.js
rename to tests/unit/models/package-info-cache/package-info-cache-test.js
index 16c16dc4d..407a8d018 100644
--- a/tests/unit/models/package-info-cache-test.js
+++ b/tests/unit/models/package-info-cache/package-info-cache-test.js
@@ -2,15 +2,15 @@
 
 const path = require('path');
 const expect = require('chai').expect;
-const PackageInfoCache = require('../../../lib/models/package-info-cache');
-const PackageInfo = require('../../../lib/models/package-info-cache/package-info');
-const Project = require('../../../lib/models/project');
-const addonFixturePath = path.resolve(__dirname, '../../fixtures/addon');
+const PackageInfoCache = require('../../../../lib/models/package-info-cache');
+const PackageInfo = require('../../../../lib/models/package-info-cache/package-info');
+const Project = require('../../../../lib/models/project');
+const addonFixturePath = path.resolve(__dirname, '../../../fixtures/addon');
 const MockUI = require('console-ui/mock');
-const MockCLI = require('../../helpers/mock-cli');
-const FixturifyProject = require('../../helpers/fixturify-project');
+const MockCLI = require('../../../helpers/mock-cli');
+const FixturifyProject = require('../../../helpers/fixturify-project');
 
-describe('models/package-info-cache.js', function() {
+describe('models/package-info-cache/package-info-cache-test.js', function() {
   let project, projectPath, packageJsonPath, packageContents, projectPackageInfo, resolvedFile, ui, cli, pic;
   this.timeout(20000);
 
@@ -100,6 +100,10 @@ describe('models/package-info-cache.js', function() {
       expect(projectPackageInfo.valid).to.be.true;
     });
 
+    it('is a project, so it may have addons', function() {
+      expect(projectPackageInfo.mayHaveAddons).to.eql(true);
+    });
+
     it('shows projectPackageInfo has cliInfo at ember-cli root dir', function() {
       expect(projectPackageInfo.cliInfo).to.exist;
 
@@ -211,6 +215,15 @@ describe('models/package-info-cache.js', function() {
         fixturifyProject.dispose();
       });
 
+      it('has dependencies who have their mayHaveAddons correctly set', function() {
+        expect(projectPackageInfo.devDependencyPackages['non-ember-thingy']).to.have.property('mayHaveAddons', false);
+        expect(projectPackageInfo.devDependencyPackages['ember-cli']).to.have.property('mayHaveAddons', false);
+        expect(projectPackageInfo.dependencyPackages['loader.js']).to.have.property('mayHaveAddons', true);
+        expect(projectPackageInfo.dependencyPackages['ember-resolver']).to.have.property('mayHaveAddons', true);
+        expect(projectPackageInfo.dependencyPackages['ember-random-addon']).to.have.property('mayHaveAddons', true);
+        expect(projectPackageInfo.dependencyPackages['something-else']).to.have.property('mayHaveAddons', true);
+      });
+
       it('validates projectPackageInfo', function() {
         expect(projectPackageInfo).to.exist;
         expect(projectPackageInfo.pkg).to.exist;