Repository: ember-cli/ember-cli
Pull request #7970 (2018-08-07 16:40:21)

Fix findAddonByName in beta.
A prior change that intended to squelch repeated warnings introduced the
following issues:

* Repeated requests for the same addon name resulted in `undefined`. For
example: `findAddonByName(arr, 'foo') !== findAddonByName(arr, 'foo')`.
* The warning suppression for unscoped matches was invalid and resulted
in repeated console output.

This fixes those issues, adds a few tests in this area, and ensures the
exact console output is also undertest.


Patch:
diff --git a/lib/models/addon.js b/lib/models/addon.js
index 6e145bb0c..f107278ce 100644
--- a/lib/models/addon.js
+++ b/lib/models/addon.js
@@ -222,13 +222,9 @@ let addonProto = {
       // The parent should already have a packageInfoCache.
       // Because it does, this package should already be in the cache.
       this.packageInfoCache = this.parent.packageInfoCache;
-    }
-
-    if (!this.packageInfoCache) {
-      // this is most likely a 'root' addon, so create a new PackageInfoCache
+    } else {
+      // this is a 'root' addon, so create a new PackageInfoCache
       // for it so we get the PackageInfo for it and its children.
-      //
-      // this may also be someone who failed to mock their addon correctly in testing.
       this.packageInfoCache = new PackageInfoCache(this.ui);
     }
 
diff --git a/lib/models/package-info-cache/package-info.js b/lib/models/package-info-cache/package-info.js
index 9e4d3d765..8b3b3891d 100644
--- a/lib/models/package-info-cache/package-info.js
+++ b/lib/models/package-info-cache/package-info.js
@@ -40,8 +40,6 @@ class PackageInfo {
     // Missing dependencies will not be considered an error, since they may
     // not actually be used.
     this.valid = true;
-
-    this._hasDumpedInvalidAddonPackages = false;
   }
 
   // Make various fields of the pkg object available.
@@ -293,9 +291,6 @@ class PackageInfo {
   }
 
   dumpInvalidAddonPackages(addonPackageList) {
-    if (this._hasDumpedInvalidAddonPackages) { return; }
-    this._hasDumpedInvalidAddonPackages = true;
-
     let invalidPackages = this.getInvalidPackages(addonPackageList);
 
     let ui = this.cache.ui;
diff --git a/lib/utilities/find-addon-by-name.js b/lib/utilities/find-addon-by-name.js
index 99e53a50c..e75a84d43 100644
--- a/lib/utilities/find-addon-by-name.js
+++ b/lib/utilities/find-addon-by-name.js
@@ -8,8 +8,8 @@ function unscope(name) {
   return name.slice(name.indexOf('/') + 1);
 }
 
-const HAS_FOUND_ADDON_BY_NAME = Object.create(null);
-const HAS_FOUND_ADDON_BY_UNSCOPED_NAME = Object.create(null);
+let HAS_FOUND_ADDON_BY_NAME = Object.create(null);
+let HAS_FOUND_ADDON_BY_UNSCOPED_NAME = Object.create(null);
 /*
   Finds an addon given a specific name. Due to older versions of ember-cli
   not properly supporting scoped packages it was (at one point) common practice
@@ -36,20 +36,32 @@ module.exports = function findAddonByName(addons, name) {
   }
 
   let exactMatchFromIndex = addons.find(addon => addon.name === name);
-  if (exactMatchFromIndex && HAS_FOUND_ADDON_BY_NAME[name] !== true) {
-    HAS_FOUND_ADDON_BY_NAME[name] = true;
-    const pkg = exactMatchFromIndex.pkg;
-    console.warn(`The addon at \`${exactMatchFromIndex.root}\` has different values in its addon index.js ('${exactMatchFromIndex.name}') and its package.json ('${pkg && pkg.name}').`);
+  if (exactMatchFromIndex) {
+    let pkg = exactMatchFromIndex.pkg;
+
+    if (HAS_FOUND_ADDON_BY_NAME[name] !== true) {
+      HAS_FOUND_ADDON_BY_NAME[name] = true;
+      console.warn(`The addon at \`${exactMatchFromIndex.root}\` has different values in its addon index.js ('${exactMatchFromIndex.name}') and its package.json ('${pkg && pkg.name}').`);
+    }
+
     return exactMatchFromIndex;
   }
 
 
   let unscopedMatchFromIndex = addons.find(addon => addon.name && unscope(addon.name) === unscopedName);
-  if (unscopedMatchFromIndex && HAS_FOUND_ADDON_BY_NAME[name] !== true) {
-    HAS_FOUND_ADDON_BY_UNSCOPED_NAME[name] = true;
-    console.trace(`Finding a scoped addon via its unscoped name is deprecated. You searched for \`${name}\` which we found as \`${unscopedMatchFromIndex.name}\` in '${unscopedMatchFromIndex.root}'`);
+  if (unscopedMatchFromIndex) {
+    if (HAS_FOUND_ADDON_BY_UNSCOPED_NAME[name] !== true) {
+      HAS_FOUND_ADDON_BY_UNSCOPED_NAME[name] = true;
+      console.trace(`Finding a scoped addon via its unscoped name is deprecated. You searched for \`${name}\` which we found as \`${unscopedMatchFromIndex.name}\` in '${unscopedMatchFromIndex.root}'`);
+    }
+
     return unscopedMatchFromIndex;
   }
 
   return null;
 };
+
+module.exports._clearCaches = function() {
+  HAS_FOUND_ADDON_BY_NAME = Object.create(null);
+  HAS_FOUND_ADDON_BY_UNSCOPED_NAME = Object.create(null);
+};


Test patch:
diff --git a/tests/unit/utilities/find-addon-by-name-test.js b/tests/unit/utilities/find-addon-by-name-test.js
index a0673553e..894b2de11 100644
--- a/tests/unit/utilities/find-addon-by-name-test.js
+++ b/tests/unit/utilities/find-addon-by-name-test.js
@@ -2,72 +2,142 @@
 
 const expect = require('chai').expect;
 const findAddonByName = require('../../../lib/utilities/find-addon-by-name');
+const clearCaches = findAddonByName._clearCaches;
 
 describe('findAddonByName', function() {
-  let addons;
+  let addons, originalConsole, consoleOutput;
+
   beforeEach(function() {
+    originalConsole = Object.assign({}, console);
+    consoleOutput = [];
+    console.warn = message => consoleOutput.push(['warn', message]);
+    console.trace = message => consoleOutput.push(['trace', message]);
+
     addons = [{
       name: 'foo',
+      root: 'node_modules/foo',
       pkg: { name: 'foo' },
     }, {
       pkg: { name: 'bar-pkg' },
+      root: 'node_modules/bar-pkg',
     }, {
       name: 'foo-bar',
+      root: 'node_modules/foo-bar',
       pkg: { name: 'foo-bar' },
     }, {
       name: '@scoped/foo-bar',
+      root: 'node_modules/@scoped/foo-bar',
       pkg: { name: '@scoped/foo-bar' },
     }, {
       name: 'thing',
+      root: 'node_modules/@scoped/thing',
       pkg: { name: '@scope/thing' },
     }, {
       name: '@scoped/other',
+      root: 'node_modules/@scoped/other',
       pkg: { name: '@scoped/other' },
     }];
   });
 
+  afterEach(function() {
+    Object.assign(console, originalConsole);
+    clearCaches();
+  });
+
   it('should return the foo addon from name', function() {
     let addon = findAddonByName(addons, 'foo');
     expect(addon.name).to.equal('foo', 'should have found the foo addon');
+    expect(consoleOutput).to.deep.equal([]);
   });
 
   it('should return the foo-bar addon from name when a foo also exists', function() {
     let addon = findAddonByName(addons, 'foo-bar');
     expect(addon.name).to.equal('foo-bar', 'should have found the foo-bar addon');
+    expect(consoleOutput).to.deep.equal([]);
   });
 
   it('should return the bar-pkg addon from package name', function() {
     let addon = findAddonByName(addons, 'bar-pkg');
     expect(addon.pkg.name).to.equal('bar-pkg', 'should have found the bar-pkg addon');
+    expect(consoleOutput).to.deep.equal([]);
   });
 
   it('should return null if addon doesn\'t exist', function() {
     let addon = findAddonByName(addons, 'not-an-addon');
     expect(addon).to.equal(null, 'not found addon should be null');
+    expect(consoleOutput).to.deep.equal([]);
   });
 
   it('should not return an addon that is a substring of requested name', function() {
     let addon = findAddonByName(addons, 'foo-ba');
     expect(addon).to.equal(null, 'foo-ba should not be found');
+    expect(consoleOutput).to.deep.equal([]);
   });
 
   it('should not guess addon name from string with slashes', function() {
     let addon = findAddonByName(addons, 'qux/foo');
     expect(addon).to.equal(null, 'should not have found the foo addon');
+    expect(consoleOutput).to.deep.equal([]);
   });
 
   it('matches scoped packages when names match exactly', function() {
     let addon = findAddonByName(addons, '@scoped/other');
     expect(addon.pkg.name).to.equal('@scoped/other');
+    expect(consoleOutput).to.deep.equal([]);
   });
 
-  it('matches unscoped name of scoped package when no exact match is found', function() {
+  it('matches unscoped name of scoped package when no exact match is found with logging', function() {
     let addon = findAddonByName(addons, 'other');
     expect(addon.pkg.name).to.equal('@scoped/other');
+    expect(consoleOutput).to.deep.equal([
+      ['trace', 'Finding a scoped addon via its unscoped name is deprecated. You searched for `other` which we found as `@scoped/other` in \'node_modules/@scoped/other\''],
+    ]);
+  });
+
+  it('matches unscoped name of scoped package repeatedly when no exact match is found with logging', function() {
+    let addon = findAddonByName(addons, 'other');
+    expect(addon.pkg.name).to.equal('@scoped/other');
+
+    addon = findAddonByName(addons, 'other');
+    expect(addon.pkg.name).to.equal('@scoped/other');
+
+    addon = findAddonByName(addons, 'other');
+    expect(addon.pkg.name).to.equal('@scoped/other');
+
+    expect(consoleOutput).to.deep.equal([
+      ['trace', 'Finding a scoped addon via its unscoped name is deprecated. You searched for `other` which we found as `@scoped/other` in \'node_modules/@scoped/other\''],
+    ]);
   });
 
   it('if exact match is found, it "wins" over unscoped matches', function() {
     let addon = findAddonByName(addons, 'foo-bar');
     expect(addon.pkg.name).to.equal('foo-bar');
+    expect(consoleOutput).to.deep.equal([]);
+  });
+
+  it('if exact match by addon name is found, it "wins" with a warning', function() {
+    let addon = findAddonByName(addons, 'thing');
+    expect(addon.pkg.name).to.equal('@scope/thing');
+    expect(consoleOutput).to.deep.equal([
+      ['warn', 'The addon at `node_modules/@scoped/thing` has different values in its addon index.js (\'thing\') and its package.json (\'@scope/thing\').'],
+    ]);
+  });
+
+  it('if exact match by addon name is found, it "wins" repeatedly', function() {
+    let addon = findAddonByName(addons, 'thing');
+    expect(addon.pkg.name).to.equal('@scope/thing');
+
+    addon = findAddonByName(addons, 'thing');
+    expect(addon.pkg.name).to.equal('@scope/thing');
+
+    addon = findAddonByName(addons, 'thing');
+    expect(addon.pkg.name).to.equal('@scope/thing');
+
+    addon = findAddonByName(addons, 'thing');
+    expect(addon.pkg.name).to.equal('@scope/thing');
+
+    expect(consoleOutput).to.deep.equal([
+      ['warn', 'The addon at `node_modules/@scoped/thing` has different values in its addon index.js (\'thing\') and its package.json (\'@scope/thing\').'],
+    ]);
   });
 });