Repository: ember-cli/ember-cli
Pull request #7401 (2017-10-25 13:45:09)

Resolve node modules correctly
ember-cli attempts to provides its own module resolution semantics which are only a subset of node's. These resolution semantics are complex and we spend many lines of code implementing them, all for no benefit over node's native resolution algorithm.

As a side effect of this behavior, we cannot take advantage of nice things like [yarn workspaces](https://github.com/ember-cli/ember-cli/issues/7335).

This PR refactors ember-cli to use complete node modules resolution (as reimplemented in userspace by the [resolve](http://npmjs.com/package/resolve) package. As far as I can tell, I have preserved all existing behavior, including the (dubious and ripe for deprecation) EMBER_NODE_PATH environment variable.

The entire `node-modules-path` package can be dropped as a dependency once we decide to stop offering the (private-but-used-by-addons) `project.nodeModulesPath` property.

I think addons should be told to use `project.require` (already public) or `project.resolveSync` (would need to be marked public) wherever they were previously using `project.nodeModulesPath`. It's just fundamentally not correct to try to encode the modules location as a single path.


Patch:
diff --git a/lib/broccoli/ember-app.js b/lib/broccoli/ember-app.js
index dac472dea..af2e3a7ad 100644
--- a/lib/broccoli/ember-app.js
+++ b/lib/broccoli/ember-app.js
@@ -288,7 +288,7 @@ class EmberApp {
 
     let emberCLIBabelInstance = this.project.findAddonByName('ember-cli-babel');
     if (emberCLIBabelInstance) {
-      let version = this.project.require('ember-cli-babel/package').version;
+      let version = this.project.require('ember-cli-babel/package.json').version;
       if (semver.lt(version, '6.0.0-alpha.1')) {
         detectedDefaultOptions.babel = {
           modules: 'amdStrict',
diff --git a/lib/commands/init.js b/lib/commands/init.js
index 650a0f187..551b3b7f4 100644
--- a/lib/commands/init.js
+++ b/lib/commands/init.js
@@ -10,7 +10,6 @@ const existsSync = require('exists-sync');
 const validProjectName = require('../utilities/valid-project-name');
 const normalizeBlueprint = require('../utilities/normalize-blueprint-option');
 const mergeBlueprintOptions = require('../utilities/merge-blueprint-options');
-const logger = require('heimdalljs-logger')('ember-cli:command:init');
 
 module.exports = Command.extend({
   name: 'init',
@@ -83,9 +82,6 @@ module.exports = Command.extend({
             verbose: commandOptions.verbose,
             useYarn: commandOptions.yarn,
             optional: false,
-          }).then(() => {
-            logger.info('setting up node_modules path for project');
-            project.setupNodeModulesPath();
           });
         }
       })
diff --git a/lib/models/addon-discovery.js b/lib/models/addon-discovery.js
index c5de51990..2b3459f06 100644
--- a/lib/models/addon-discovery.js
+++ b/lib/models/addon-discovery.js
@@ -8,7 +8,6 @@ const logger = require('heimdalljs-logger')('ember-cli:addon-discovery');
 const existsSync = require('exists-sync');
 const path = require('path');
 const resolve = require('resolve');
-const findup = require('find-up');
 const heimdall = require('heimdalljs');
 
 /**
@@ -44,11 +43,7 @@ class AddonDiscovery {
     let cliAddons = this.discoverFromCli(project.cli);
     let dependencyAddons;
 
-    if (project.hasDependencies()) {
-      dependencyAddons = this.discoverFromDependencies(project.root, project.nodeModulesPath, project.pkg, false);
-    } else {
-      dependencyAddons = [];
-    }
+    dependencyAddons = this.discoverFromDependencies(project.root, project.pkg, false);
 
     let inRepoAddons = this.discoverInRepoAddons(project.root, project.pkg);
     let addons = projectAsAddon.concat(cliAddons, internalAddons, dependencyAddons, inRepoAddons);
@@ -73,7 +68,7 @@ class AddonDiscovery {
       addonDiscoveryNode: true,
     });
     logger.info('discoverChildAddons: %s(%s)', addon.name, addon.root);
-    let dependencyAddons = this.discoverFromDependencies(addon.root, addon.nodeModulesPath, addon.pkg, true);
+    let dependencyAddons = this.discoverFromDependencies(addon.root, addon.pkg, true);
     let inRepoAddons = this.discoverInRepoAddons(addon.root, addon.pkg);
     let addons = dependencyAddons.concat(inRepoAddons);
 
@@ -108,7 +103,7 @@ class AddonDiscovery {
     @private
     @method discoverFromDependencies
    */
-  discoverFromDependencies(root, nodeModulesPath, pkg, excludeDevDeps) {
+  discoverFromDependencies(root, pkg, excludeDevDeps) {
     let addons = Object.keys(this.dependencies(pkg, excludeDevDeps)).map(name => {
       if (name !== 'ember-cli') {
         let addonPath = this.resolvePackage(root, name);
@@ -116,18 +111,6 @@ class AddonDiscovery {
         if (addonPath) {
           return this.discoverAtPath(addonPath);
         }
-
-        // this supports packages that do not have a valid entry point
-        // script (aka `main` entry in `package.json` or `index.js`)
-        addonPath = path.join(nodeModulesPath, name);
-        let addon = this.discoverAtPath(addonPath);
-        if (addon) {
-          const chalk = require('chalk');
-
-          this.ui.writeLine(chalk.yellow(`The package \`${name}\` is not a properly formatted package, we have used a fallback lookup to resolve it at \`${addonPath}\`. This is generally caused by an addon not having a \`main\` entry point (or \`index.js\`).`), 'WARNING');
-
-          return addon;
-        }
       }
     }).filter(Boolean);
 
@@ -136,13 +119,9 @@ class AddonDiscovery {
 
   resolvePackage(root, packageName) {
     try {
-
-      let entryModulePath = resolve.sync(packageName, { basedir: root });
-
-      let pkgPath = findup.sync('package.json', { cwd: entryModulePath });
-      if (pkgPath) {
-        return path.dirname(pkgPath);
-      }
+      // Packages are not required to have a valid Javascript entrypoint, but they are required to have a package.json.
+      let pkgPath = resolve.sync(path.join(packageName, 'package.json'), { basedir: root });
+      return path.dirname(pkgPath);
 
     } catch (e) {
       if (e.code === 'MODULE_NOT_FOUND') {
diff --git a/lib/models/addon.js b/lib/models/addon.js
index 5bfffae04..dc996438a 100644
--- a/lib/models/addon.js
+++ b/lib/models/addon.js
@@ -12,8 +12,6 @@ const logger = heimdallLogger('ember-cli:addon');
 const treeCacheLogger = heimdallLogger('ember-cli:addon:tree-cache');
 const cacheKeyLogger = heimdallLogger('ember-cli:addon:cache-key-for-tree');
 
-const nodeModulesPath = require('node-modules-path');
-
 const p = require('ember-cli-preprocess-registry/preprocessors');
 const preprocessJs = p.preprocessJs;
 const preprocessCss = p.preprocessCss;
@@ -207,7 +205,7 @@ let addonProto = {
     if (!this.root) {
       throw new Error('Addon classes must be instantiated with the `root` property');
     }
-    this.nodeModulesPath = nodeModulesPath(this.root);
+    this._nodeModulesPath = null;
 
     this.treePaths = {
       app: 'app',
@@ -1670,6 +1668,27 @@ Addon.prototype[BUILD_BABEL_OPTIONS_FOR_PREPROCESSORS] = function() {
   }
 };
 
+Object.defineProperty(Addon.prototype, 'nodeModulesPath', {
+  // This exists only for backward compat, because some addons use
+  // it. It was never marked public and we can totally nuke it, but we
+  // can be polite and give them a warning for a while.
+  get() {
+    if (!this._nodeModulesPath) {
+      let nodeModulesPath = require('node-modules-path');
+      let value = nodeModulesPath(this.root);
+      this._nodeModulesPath = { value };
+
+      let stack = new Error().stack;
+      stack = stack.split('\n')[2];
+      stack = stack.replace('    at ', '  ');
+
+      this.ui.writeDeprecateLine(`An addon is trying to access addon.nodeModulesPath. This is not a reliable way to discover npm modules. Instead, consider using require("resolve").sync(something, { basedir: addon.root }). Accessed from: ${stack.toString()}`);
+    }
+    return this._nodeModulesPath.value;
+  },
+});
+
+
 /**
   Returns the absolute path for a given addon
 
diff --git a/lib/models/installation-checker.js b/lib/models/installation-checker.js
index f1589d079..cfec08b53 100644
--- a/lib/models/installation-checker.js
+++ b/lib/models/installation-checker.js
@@ -59,7 +59,7 @@ class InstallationChecker {
   }
 
   npmDependenciesNotPresent() {
-    return !existsSync(this.project.nodeModulesPath);
+    return !this.project.hasDependencies();
   }
 }
 
@@ -77,4 +77,3 @@ function readJSON(path) {
     throw new SilentError(`InstallationChecker: Unable to parse: ${path}`);
   }
 }
-
diff --git a/lib/models/project.js b/lib/models/project.js
index 850da0b7a..a50d6d884 100644
--- a/lib/models/project.js
+++ b/lib/models/project.js
@@ -11,7 +11,6 @@ const fs = require('fs-extra');
 const existsSync = require('exists-sync');
 const _ = require('ember-cli-lodash-subset');
 const logger = require('heimdalljs-logger')('ember-cli:project');
-const nodeModulesPath = require('node-modules-path');
 const versionUtils = require('../utilities/version-utils');
 const emberCLIVersion = versionUtils.emberCLIVersion;
 const findAddonByName = require('../utilities/find-addon-by-name');
@@ -50,7 +49,6 @@ class Project {
     this.addons = [];
     this.liveReloadFilterPatterns = [];
     this.setupBowerDirectory();
-    this.setupNodeModulesPath();
     this.addonDiscovery = new AddonDiscovery(this.ui);
     this.addonsFactory = new AddonsFactory(this, this);
     this.configCache = new Map();
@@ -79,6 +77,7 @@ class Project {
     instrumentation.project = this;
 
     this.emberCLIVersion = emberCLIVersion;
+    this._nodeModulesPath = null;
   }
 
   /**
@@ -105,23 +104,49 @@ class Project {
     logger.info('bowerDirectory: %s', this.bowerDirectory);
   }
 
-  hasDependencies() {
-    return !!this.nodeModulesPath;
-  }
+  // This exists only for backward compat, because some addons use
+  // it. It was never marked public and we can totally nuke it, but we
+  // can be polite and give them a warning for a while.
+  get nodeModulesPath() {
+    if (!this._nodeModulesPath) {
+      let nodeModulesPath = require('node-modules-path');
+      let value = nodeModulesPath(this.root);
+      this._nodeModulesPath = { value };
 
-  /**
-    Sets the path to the node_modules directory for this
-    project.
+      let stack = new Error().stack;
+      stack = stack.split('\n')[2];
+      stack = stack.replace('    at ', '  ');
 
-    @private
-    @method setupNodeModulesPath
-   */
-  setupNodeModulesPath() {
-    this.nodeModulesPath = nodeModulesPath(this.root);
+      this.ui.writeDeprecateLine(`An addon is trying to access project.nodeModulesPath. This is not a reliable way to discover npm modules. Instead, consider doing: require("resolve").sync(something, { basedir: project.root }). Accessed from: ${stack.toString()}`);
+    }
+    return this._nodeModulesPath.value;
+  }
 
-    logger.info('nodeModulesPath: %s', this.nodeModulesPath);
+  // Checks whether the project's npm dependencies are
+  // present. Previously this just looked for a node_modules folder in
+  // a fixed place (which is not compatible with node's module
+  // resolution algorithm). Now we just sample to see if we can
+  // resolve the first dependency or devDependency we find.
+  hasDependencies() {
+    if (this.cli.disableDependencyChecker) {
+      // Blueprint tests set this flag.
+      return true;
+    }
+    for (let depName in this.dependencies()) {
+      try {
+        this.resolveSync(depName);
+        return true;
+      } catch (err) {
+        if (err.code !== 'MODULE_NOT_FOUND') {
+          throw err;
+        }
+        return false;
+      }
+    }
+    return false;
   }
 
+
   static nullProject(ui, cli) {
     if (NULL_PROJECT) { return NULL_PROJECT; }
 
@@ -299,20 +324,6 @@ class Project {
     return existsSync(path.join(this.root, file)) || existsSync(path.join(this.root, `${file}.js`));
   }
 
-  /**
-    Resolves the absolute path to a file.
-
-    @private
-    @method resolve
-    @param  {String} file File to resolve
-    @return {String}      Absolute path to file
-   */
-  resolve(file) {
-    return resolve(file, {
-      basedir: this.root,
-    });
-  }
-
   /**
     Resolves the absolute path to a file synchronously
 
@@ -323,7 +334,7 @@ class Project {
    */
   resolveSync(file) {
     return resolve.sync(file, {
-      basedir: this.root,
+      basedir: process.env.EMBER_NODE_PATH || this.root,
     });
   }
 
@@ -338,13 +349,8 @@ class Project {
     @return {Object}      Imported module
    */
   require(file) {
-    if (/^\.\//.test(file)) { // Starts with ./
-      return require(path.join(this.root, file));
-    } else if (file.slice(0, this.root.length) === this.root) { // Starts with this.root
-      return require(file);
-    } else {
-      return require(path.join(this.nodeModulesPath, file));
-    }
+    let path = this.resolveSync(file);
+    return require(path);
   }
 
   /**
diff --git a/lib/utilities/require-local.js b/lib/utilities/require-local.js
deleted file mode 100644
index 1608d4490..000000000
--- a/lib/utilities/require-local.js
+++ /dev/null
@@ -1,8 +0,0 @@
-'use strict';
-
-const path = require('path');
-const nodeModulesPath = require('node-modules-path');
-
-module.exports = function requireLocal(lib) {
-  return require(path.join(nodeModulesPath(process.cwd()), lib));
-};


Test patch:
diff --git a/tests/unit/models/addon-discovery-test.js b/tests/unit/models/addon-discovery-test.js
index 64ed525af..a354b2924 100644
--- a/tests/unit/models/addon-discovery-test.js
+++ b/tests/unit/models/addon-discovery-test.js
@@ -7,7 +7,6 @@ const AddonDiscovery = require('../../../lib/models/addon-discovery');
 let fixturePath = path.resolve(__dirname, '../../fixtures/addon');
 const MockUI = require('console-ui/mock');
 const MockCLI = require('../../helpers/mock-cli');
-const chalk = require('chalk');
 
 describe('models/addon-discovery.js', function() {
   let project, projectPath, ui;
@@ -186,7 +185,6 @@ describe('models/addon-discovery.js', function() {
 
     it('can find a package without a main entry point [DEPRECATED]', function() {
       let root = path.join(fixturePath, 'shared-package', 'base');
-      let addonNodeModulesPath = path.join(root, 'node_modules');
       let actualPaths = [];
       let discovery = new AddonDiscovery(ui);
 
@@ -197,7 +195,7 @@ describe('models/addon-discovery.js', function() {
         return providedPath;
       };
 
-      discovery.discoverFromDependencies(root, addonNodeModulesPath, mockPkg, true);
+      discovery.discoverFromDependencies(root, mockPkg, true);
 
       let expectedPaths = [
         path.join(root, 'node_modules', 'foo-bar'),
@@ -206,15 +204,10 @@ describe('models/addon-discovery.js', function() {
       ];
 
       expect(actualPaths).to.deep.equal(expectedPaths);
-
-      let output = ui.output.trim();
-      let expectedWarning = chalk.yellow(`The package \`invalid-package\` is not a properly formatted package, we have used a fallback lookup to resolve it at \`${path.join(root, 'node_modules', 'invalid-package')}\`. This is generally caused by an addon not having a \`main\` entry point (or \`index.js\`).`);
-      expect(output).to.equal(expectedWarning);
     });
 
     it('does not error when dependencies are not found', function() {
       let root = path.join(fixturePath, 'shared-package', 'base');
-      let addonNodeModulesPath = path.join(root, 'node_modules');
       let actualPaths = [];
       let discovery = new AddonDiscovery(ui);
 
@@ -225,12 +218,11 @@ describe('models/addon-discovery.js', function() {
         return providedPath;
       };
 
-      discovery.discoverFromDependencies(root, addonNodeModulesPath, mockPkg, true);
+      discovery.discoverFromDependencies(root, mockPkg, true);
 
       let expectedPaths = [
         path.join(root, 'node_modules', 'foo-bar'),
         path.join(root, 'node_modules', 'blah-blah'),
-        path.join(root, 'node_modules', 'blah-zorz'),
       ];
 
       expect(actualPaths).to.deep.equal(expectedPaths);
@@ -238,7 +230,6 @@ describe('models/addon-discovery.js', function() {
 
     it('calls discoverAtPath for each entry in dependencies', function() {
       let root = path.join(fixturePath, 'shared-package', 'base');
-      let addonNodeModulesPath = path.join(root, 'node_modules');
       let actualPaths = [];
       let discovery = new AddonDiscovery(ui);
 
@@ -248,7 +239,7 @@ describe('models/addon-discovery.js', function() {
         return providedPath;
       };
 
-      discovery.discoverFromDependencies(root, addonNodeModulesPath, mockPkg);
+      discovery.discoverFromDependencies(root, mockPkg);
 
       let expectedPaths = [
         path.join(root, '..', 'node_modules', 'dev-foo-bar'),
@@ -261,7 +252,6 @@ describe('models/addon-discovery.js', function() {
 
     it('excludes devDeps if `excludeDevDeps` is true', function() {
       let root = path.join(fixturePath, 'shared-package', 'base');
-      let addonNodeModulesPath = path.join(root, 'node_modules');
       let actualPaths = [];
       let discovery = new AddonDiscovery(ui);
 
@@ -271,7 +261,7 @@ describe('models/addon-discovery.js', function() {
         return providedPath;
       };
 
-      discovery.discoverFromDependencies(root, addonNodeModulesPath, mockPkg, true);
+      discovery.discoverFromDependencies(root, mockPkg, true);
 
       let expectedPaths = [
         path.join(root, 'node_modules', 'foo-bar'),
diff --git a/tests/unit/models/installation-checker-test.js b/tests/unit/models/installation-checker-test.js
index 4102cdc3d..d39640fb9 100644
--- a/tests/unit/models/installation-checker-test.js
+++ b/tests/unit/models/installation-checker-test.js
@@ -44,7 +44,6 @@ describe('Installation Checker', function() {
     it('works when installation directory exist', function() {
       let project = {
         root: fixturePath('installation-checker/valid-npm-installation'),
-        nodeModulesPath: fixturePath('installation-checker/valid-npm-installation/node_modules'),
       };
       installationChecker = new InstallationChecker({ project });
 
@@ -54,7 +53,6 @@ describe('Installation Checker', function() {
     it('fails when installation directory doesn\'t exist', function() {
       let project = {
         root: fixturePath('installation-checker/invalid-npm-installation'),
-        nodeModulesPath: fixturePath('installation-checker/invalid-npm-installation/node_modules'),
       };
       installationChecker = new InstallationChecker({ project });
 
@@ -69,7 +67,6 @@ describe('Installation Checker', function() {
       let project = {
         root: fixturePath('installation-checker/invalid-bower-and-npm'),
         bowerDirectory: fixturePath('installation-checker/invalid-bower-and-npm/bower_components'),
-        nodeModulesPath: fixturePath('installation-checker/invalid-bower-and-npm/node_modules'),
       };
       installationChecker = new InstallationChecker({ project });
 
@@ -80,7 +77,6 @@ describe('Installation Checker', function() {
       let project = {
         root: fixturePath('installation-checker/empty'),
         bowerDirectory: fixturePath('installation-checker/empty/bower_components'),
-        nodeModulesPath: fixturePath('installation-checker/empty/node_modules'),
       };
       installationChecker = new InstallationChecker({ project });