Repository: ember-cli/ember-cli
Pull request #9542 (2021-05-24 16:46:14)

Add validation checks for addon instance bundle caching
This PR implements two validation checks for addon bundle caching:

1. If an `EmberApp` has a project that has opted in to addon bundle caching _and_ that app has utilized the addon `whitelist`/`blacklist` this is treated as an error condition. Addon bundle caching needs to know the host addons _during_ addon instantiation, but these configurations change these semantics afterwards, so we're treating this as an error condition.
2. If a proxy _doesn't_ return stable cache keys for a given tree type, this is treated as an error condition. Bundle addon caching is assumed to be used on addons w/ stable cache keys (eg, for this to work properly w/ `ember-engines` the cache key needs to be stable).

I've also added tests to cover these error conditions.


Patch:
diff --git a/lib/broccoli/ember-app.js b/lib/broccoli/ember-app.js
index 833473738..0b52fe442 100644
--- a/lib/broccoli/ember-app.js
+++ b/lib/broccoli/ember-app.js
@@ -168,6 +168,30 @@ class EmberApp {
 
     this._isPackageHookSupplied = typeof this.options.package === 'function';
     this._cachedAddonBundles = {};
+
+    if (this.project.perBundleAddonCache && this.project.perBundleAddonCache.numProxies > 0) {
+      if (this.options.addons.whitelist && this.options.addons.whitelist.length) {
+        throw new Error(
+          [
+            '[ember-cli] addon bundle caching is disabled for apps that specify an addon `whitelist`',
+            '',
+            'All addons using bundle caching:',
+            ...this.project.perBundleAddonCache.getPathsToAddonsOptedIn(),
+          ].join('\n')
+        );
+      }
+
+      if (this.options.addons.blacklist && this.options.addons.blacklist.length) {
+        throw new Error(
+          [
+            '[ember-cli] addon bundle caching is disabled for apps that specify an addon `blacklist`',
+            '',
+            'All addons using bundle caching:',
+            ...this.project.perBundleAddonCache.getPathsToAddonsOptedIn(),
+          ].join('\n')
+        );
+      }
+    }
   }
 
   /**
diff --git a/lib/models/per-bundle-addon-cache/addon-proxy.js b/lib/models/per-bundle-addon-cache/addon-proxy.js
index a1b5612b6..9c41b3c7d 100644
--- a/lib/models/per-bundle-addon-cache/addon-proxy.js
+++ b/lib/models/per-bundle-addon-cache/addon-proxy.js
@@ -2,6 +2,43 @@
 
 const { TARGET_INSTANCE } = require('./target-instance');
 
+const CACHE_KEY_FOR_TREE_TRACKER = Symbol('CACHE_KEY_FOR_TREE_TRACKER');
+
+/**
+ * Validates that a new cache key for a given tree type matches the previous
+ * cache key for the same tree type. To opt-in to bundle addon caching for
+ * a given addon it's assumed that it returns stable cache keys; specifically
+ * this is because the interplay between bundle addon caching and `ember-engines`
+ * when transitive deduplication is enabled assumes stable cache keys, so we validate
+ * for this case here.
+ *
+ * @method validateCacheKey
+ * @param {Addon} realAddonInstance The real addon instance
+ * @param {string} treeType
+ * @param {string} newCacheKey
+ * @throws {Error} If the new cache key doesn't match the previous cache key
+ */
+function validateCacheKey(realAddonInstance, treeType, newCacheKey) {
+  let cacheKeyTracker = realAddonInstance[CACHE_KEY_FOR_TREE_TRACKER];
+
+  if (!cacheKeyTracker) {
+    cacheKeyTracker = {};
+    realAddonInstance[CACHE_KEY_FOR_TREE_TRACKER] = cacheKeyTracker;
+  }
+
+  cacheKeyTracker[treeType] = treeType in cacheKeyTracker ? cacheKeyTracker[treeType] : newCacheKey;
+
+  if (cacheKeyTracker[treeType] !== newCacheKey) {
+    throw new Error(
+      `[ember-cli] addon bundle caching can only be used on addons that have stable cache keys (previously \`${
+        cacheKeyTracker[treeType]
+      }\`, now \`${newCacheKey}\`; for addon \`${realAddonInstance.name}\` located at \`${
+        realAddonInstance.packageRoot || realAddonInstance.root
+      }\`)`
+    );
+  }
+}
+
 /**
  * Returns a proxy to a target with specific handling for the
  * `parent` property, as well has to handle the `app` property;
@@ -26,7 +63,8 @@ const { TARGET_INSTANCE } = require('./target-instance');
  */
 function getAddonProxy(targetCacheEntry, parent) {
   let _app;
-  let addonProxy = new Proxy(targetCacheEntry, {
+
+  return new Proxy(targetCacheEntry, {
     get(targetCacheEntry, property) {
       if (property === 'parent') {
         return parent;
@@ -64,7 +102,14 @@ function getAddonProxy(targetCacheEntry, parent) {
           // invoked is the proxy, not the original instance (so its local state is incorrect).
           // Wrap the original methods to maintain the correct 'this' context.
           return function _originalAddonPropMethodWrapper() {
-            return targetCacheEntry[TARGET_INSTANCE][property](...arguments);
+            let originalReturnValue = targetCacheEntry[TARGET_INSTANCE][property](...arguments);
+
+            if (property === 'cacheKeyForTree') {
+              const treeType = arguments[0];
+              validateCacheKey(targetCacheEntry[TARGET_INSTANCE], treeType, originalReturnValue);
+            }
+
+            return originalReturnValue;
           };
         }
 
@@ -86,7 +131,6 @@ function getAddonProxy(targetCacheEntry, parent) {
       return Reflect.set(targetCacheEntry, property, value);
     },
   });
-
-  return addonProxy;
 }
+
 module.exports = { getAddonProxy };
diff --git a/lib/models/per-bundle-addon-cache/index.js b/lib/models/per-bundle-addon-cache/index.js
index 2f80da8d9..b3f05b5fa 100644
--- a/lib/models/per-bundle-addon-cache/index.js
+++ b/lib/models/per-bundle-addon-cache/index.js
@@ -300,6 +300,18 @@ class PerBundleAddonCache {
     }
   }
 
+  getPathsToAddonsOptedIn() {
+    const addonSet = new Set();
+
+    for (const [, { addonInstanceCache }] of this.bundleHostCache) {
+      Array.from(addonInstanceCache.keys()).forEach((realPath) => {
+        addonSet.add(realPath);
+      });
+    }
+
+    return Array.from(addonSet);
+  }
+
   _getBundleHostCacheEntry(pkgInfo) {
     let cacheEntry = this.bundleHostCache.get(pkgInfo);
 
@@ -310,15 +322,15 @@ class PerBundleAddonCache {
 
     return cacheEntry;
   }
+
+  // Support for per-bundle addon caching is GLOBAL opt OUT (unless you explicitly set
+  // EMBER_CLI_ADDON_INSTANCE_CACHING to false, it will be enabled.) If you opt out, that
+  // overrides setting `allowCachingPerBundle` for any particular addon type to true.
+  // To help make testing easier, we'll expose the setting as a function so it can be
+  // called multiple times and evaluate each time.
+  static isEnabled() {
+    return process.env.EMBER_CLI_ADDON_INSTANCE_CACHING !== 'false';
+  }
 }
 
 module.exports = PerBundleAddonCache;
-
-// Support for per-bundle addon caching is GLOBAL opt OUT (unless you explicitly set
-// EMBER_CLI_ADDON_INSTANCE_CACHING to false, it will be enabled.) If you opt out, that
-// overrides setting `allowCachingPerBundle` for any particular addon type to true.
-// To help make testing easier, we'll expose the setting as a function so it can be
-// called multiple times and evaluate each time.
-module.exports.isEnabled = function () {
-  return process.env.EMBER_CLI_ADDON_INSTANCE_CACHING !== 'false';
-};


Test patch:
diff --git a/tests/unit/broccoli/ember-app-test.js b/tests/unit/broccoli/ember-app-test.js
index a982a63a3..65673a1f8 100644
--- a/tests/unit/broccoli/ember-app-test.js
+++ b/tests/unit/broccoli/ember-app-test.js
@@ -1,6 +1,7 @@
 'use strict';
 
 const path = require('path');
+const FixturifyProject = require('../../helpers/fixturify-project');
 const Project = require('../../../lib/models/project');
 const expect = require('chai').expect;
 const td = require('testdouble');
@@ -1221,6 +1222,79 @@ describe('EmberApp', function () {
       });
     });
 
+    describe('addon instance bundle caching validation (when used within the project)', function () {
+      let fixturifyProject;
+
+      beforeEach(function () {
+        fixturifyProject = new FixturifyProject('awesome-proj', '1.0.0');
+        fixturifyProject.addDevDependency('ember-cli', '*');
+      });
+
+      afterEach(function () {
+        fixturifyProject.dispose();
+      });
+
+      it('throws an error if an addon `whitelist` is specified', function () {
+        fixturifyProject.addInRepoAddon('foo', '1.0.0', { allowCachingPerBundle: true });
+        fixturifyProject.addInRepoAddon('foo-bar', '1.0.0', {
+          callback: (inRepoAddon) => {
+            inRepoAddon.pkg['ember-addon'].paths = ['../foo'];
+          },
+        });
+
+        fixturifyProject.writeSync();
+
+        let projectWithBundleCaching = fixturifyProject.buildProjectModel();
+        projectWithBundleCaching.initializeAddons();
+
+        expect(() => {
+          new EmberApp({
+            project: projectWithBundleCaching,
+            addons: {
+              whitelist: ['foo'],
+            },
+          });
+        }).to.throw(
+          [
+            '[ember-cli] addon bundle caching is disabled for apps that specify an addon `whitelist`',
+            '',
+            'All addons using bundle caching:',
+            projectWithBundleCaching.addons.find((addon) => addon.name === 'foo').packageRoot,
+          ].join('\n')
+        );
+      });
+
+      it('throws an error if an addon `blacklist` is specified', function () {
+        fixturifyProject.addInRepoAddon('foo', '1.0.0', { allowCachingPerBundle: true });
+        fixturifyProject.addInRepoAddon('foo-bar', '1.0.0', {
+          callback: (inRepoAddon) => {
+            inRepoAddon.pkg['ember-addon'].paths = ['../foo'];
+          },
+        });
+
+        fixturifyProject.writeSync();
+
+        let projectWithBundleCaching = fixturifyProject.buildProjectModel();
+        projectWithBundleCaching.initializeAddons();
+
+        expect(() => {
+          new EmberApp({
+            project: projectWithBundleCaching,
+            addons: {
+              blacklist: ['foo'],
+            },
+          });
+        }).to.throw(
+          [
+            '[ember-cli] addon bundle caching is disabled for apps that specify an addon `blacklist`',
+            '',
+            'All addons using bundle caching:',
+            projectWithBundleCaching.addons.find((addon) => addon.name === 'foo').packageRoot,
+          ].join('\n')
+        );
+      });
+    });
+
     describe('addonLintTree', function () {
       beforeEach(function () {
         addon = {
diff --git a/tests/unit/models/per-bundle-addon-cache/proxy-test.js b/tests/unit/models/per-bundle-addon-cache/proxy-test.js
index 8a69fe1ff..6be9aa2c0 100644
--- a/tests/unit/models/per-bundle-addon-cache/proxy-test.js
+++ b/tests/unit/models/per-bundle-addon-cache/proxy-test.js
@@ -5,7 +5,8 @@
  * its addons
  */
 const expect = require('chai').expect;
-const FixturifyProject = require('../../../../tests/helpers/fixturify-project');
+const path = require('path');
+const FixturifyProject = require('../../../helpers/fixturify-project');
 
 const {
   findAddonCacheEntriesByName,
@@ -1012,4 +1013,73 @@ describe('models/per-bundle-addon-cache', function () {
       expect(areAllInstancesEqualWithinHost(project, 'lazy-engine-a')).to.be.true;
     });
   });
+
+  describe('validation', function () {
+    function findAllAddonsByName(projectOrAddon, addonToFind, _foundAddons = []) {
+      projectOrAddon.addons.forEach((addon) => {
+        if (addon.name === addonToFind) {
+          _foundAddons.push(addon);
+        }
+
+        findAllAddonsByName(addon, addonToFind, _foundAddons);
+      });
+
+      return _foundAddons;
+    }
+
+    it('does not throw if the addon returns a stable cache key', function () {
+      fixturifyProject.addInRepoAddon('foo', '1.0.0', {
+        allowCachingPerBundle: true,
+      });
+
+      fixturifyProject.addInRepoAddon('foo-bar', '1.0.0', {
+        callback: (inRepoAddon) => {
+          inRepoAddon.pkg['ember-addon'].paths = ['../foo'];
+        },
+      });
+
+      fixturifyProject.writeSync();
+      let project = fixturifyProject.buildProjectModel();
+
+      project.initializeAddons();
+
+      findAllAddonsByName(project, 'foo').forEach((addon) => {
+        addon.cacheKeyForTree();
+        addon.cacheKeyForTree();
+
+        addon.cacheKeyForTree('addon');
+        addon.cacheKeyForTree('addon');
+      });
+    });
+
+    it('throws an error for addons that do not have stable cache keys', function () {
+      fixturifyProject.addInRepoAddon('foo', '1.0.0', {
+        allowCachingPerBundle: true,
+        additionalContent: `init() {this._super.init.apply(this, arguments); this.current = 0;}, cacheKeyForTree() {return this.current++;},`,
+      });
+
+      fixturifyProject.addInRepoAddon('foo-bar', '1.0.0', {
+        callback: (inRepoAddon) => {
+          inRepoAddon.pkg['ember-addon'].paths = ['../foo'];
+        },
+      });
+
+      fixturifyProject.writeSync();
+      let project = fixturifyProject.buildProjectModel();
+
+      project.initializeAddons();
+
+      expect(() => {
+        findAllAddonsByName(project, 'foo').forEach((addon) => {
+          addon.cacheKeyForTree();
+          addon.cacheKeyForTree();
+        });
+      }).to.throw(
+        `[ember-cli] addon bundle caching can only be used on addons that have stable cache keys (previously \`2\`, now \`3\`; for addon \`foo\` located at \`${path.join(
+          fixturifyProject.baseDir,
+          'lib/foo'
+        )}\`)`
+      );
+    });
+  });
 });