[Fix] sync/async: fix preserveSymlinks option

Fixes #177.

Author: ljharb

.gitignore

@@ -1,5 +1,6 @@
 # gitignore
 node_modules
+**/node_modules
 
 # Only apps should have lockfiles
 npm-shrinkwrap.json

.travis.yml

@@ -35,6 +35,8 @@ matrix:
   include:
     - node_js: "lts/*"
       env: PRETEST=true
+    - node_js: "lts/*"
+      env: POSTTEST=true
     - node_js: "11.3"
       env: TEST=true ALLOW_FAILURE=true
     - node_js: "11.2"

lib/async.js

@@ -40,22 +40,37 @@ module.exports = function resolve(x, options, callback) {
 
     opts.paths = opts.paths || [];
 
-    if ((/^(?:\.\.?(?:\/|$)|\/|([A-Za-z]:)?[/\\])/).test(x)) {
-        var res = path.resolve(basedir, x);
-        if (x === '..' || x.slice(-1) === '/') res += '/';
-        if ((/\/$/).test(x) && res === basedir) {
-            loadAsDirectory(res, opts.package, onfile);
-        } else loadAsFile(res, opts.package, onfile);
-    } else loadNodeModules(x, basedir, function (err, n, pkg) {
-        if (err) cb(err);
-        else if (n) cb(null, n, pkg);
-        else if (core[x]) return cb(null, x);
-        else {
-            var moduleError = new Error("Cannot find module '" + x + "' from '" + parent + "'");
-            moduleError.code = 'MODULE_NOT_FOUND';
-            cb(moduleError);
-        }
-    });
+    // ensure that `basedir` is an absolute path at this point, resolving against the process' current working directory
+    var absoluteStart = path.resolve(basedir);
+
+    if (opts.preserveSymlinks === false) {
+        fs.realpath(absoluteStart, function (realPathErr, realStart) {
+            if (realPathErr && realPathErr.code !== 'ENOENT') cb(err);
+            else init(realStart);
+        });
+    } else {
+        init(absoluteStart);
+    }
+
+    var res;
+    function init(basedir) {
+        if ((/^(?:\.\.?(?:\/|$)|\/|([A-Za-z]:)?[/\\])/).test(x)) {
+            res = path.resolve(basedir, x);
+            if (x === '..' || x.slice(-1) === '/') res += '/';
+            if ((/\/$/).test(x) && res === basedir) {
+                loadAsDirectory(res, opts.package, onfile);
+            } else loadAsFile(res, opts.package, onfile);
+        } else loadNodeModules(x, basedir, function (err, n, pkg) {
+            if (err) cb(err);
+            else if (n) cb(null, n, pkg);
+            else if (core[x]) return cb(null, x);
+            else {
+                var moduleError = new Error("Cannot find module '" + x + "' from '" + parent + "'");
+                moduleError.code = 'MODULE_NOT_FOUND';
+                cb(moduleError);
+            }
+        });
+    }
 
     function onfile(err, m, pkg) {
         if (err) cb(err);

lib/node-modules-paths.js

@@ -1,5 +1,4 @@
 var path = require('path');
-var fs = require('fs');
 var parse = path.parse || require('path-parse');
 
 var getNodeModulesDirs = function getNodeModulesDirs(absoluteStart, modules) {
@@ -29,28 +28,15 @@ module.exports = function nodeModulesPaths(start, opts, request) {
         ? [].concat(opts.moduleDirectory)
         : ['node_modules'];
 
-    // ensure that `start` is an absolute path at this point, resolving against the process' current working directory
-    var absoluteStart = path.resolve(start);
-
-    if (opts && opts.preserveSymlinks === false) {
-        try {
-            absoluteStart = fs.realpathSync(absoluteStart);
-        } catch (err) {
-            if (err.code !== 'ENOENT') {
-                throw err;
-            }
-        }
-    }
-
     if (opts && typeof opts.paths === 'function') {
         return opts.paths(
             request,
-            absoluteStart,
-            function () { return getNodeModulesDirs(absoluteStart, modules); },
+            start,
+            function () { return getNodeModulesDirs(start, modules); },
             opts
         );
     }
 
-    var dirs = getNodeModulesDirs(absoluteStart, modules);
+    var dirs = getNodeModulesDirs(start, modules);
     return opts && opts.paths ? dirs.concat(opts.paths) : dirs;
 };

lib/sync.js

@@ -30,13 +30,26 @@ module.exports = function (x, options) {
 
     opts.paths = opts.paths || [];
 
+    // ensure that `basedir` is an absolute path at this point, resolving against the process' current working directory
+    var absoluteStart = path.resolve(basedir);
+
+    if (opts.preserveSymlinks === false) {
+        try {
+            absoluteStart = fs.realpathSync(absoluteStart);
+        } catch (realPathErr) {
+            if (realPathErr.code !== 'ENOENT') {
+                throw realPathErr;
+            }
+        }
+    }
+
     if ((/^(?:\.\.?(?:\/|$)|\/|([A-Za-z]:)?[/\\])/).test(x)) {
-        var res = path.resolve(basedir, x);
+        var res = path.resolve(absoluteStart, x);
         if (x === '..' || x.slice(-1) === '/') res += '/';
         var m = loadAsFileSync(res) || loadAsDirectorySync(res);
         if (m) return m;
     } else {
-        var n = loadNodeModulesSync(x, basedir);
+        var n = loadNodeModulesSync(x, absoluteStart);
         if (n) return n;
     }
 

package.json

@@ -18,7 +18,9 @@
     "lint": "eslint .",
     "tests-only": "tape test/*.js",
     "pretest": "npm run lint",
-    "test": "npm run --silent tests-only"
+    "test": "npm run --silent tests-only",
+    "posttest": "npm run test:multirepo",
+    "test:multirepo": "cd ./test/resolver/multirepo && npm install && npm test"
   },
   "devDependencies": {
     "@ljharb/eslint-config": "^13.0.0",

test/node_path.js

@@ -1,16 +1,31 @@
+var fs = require('fs');
 var path = require('path');
 var test = require('tape');
 var resolve = require('../');
 
 test('$NODE_PATH', function (t) {
     t.plan(8);
 
+    var isDir = function (dir, cb) {
+        if (dir === '/node_path' || dir === 'node_path/x') {
+            return cb(null, true);
+        }
+        fs.stat(dir, function (err, stat) {
+            if (!err) {
+                return cb(null, stat.isDirectory());
+            }
+            if (err.code === 'ENOENT' || err.code === 'ENOTDIR') return cb(null, false);
+            return cb(err);
+        });
+    };
+
     resolve('aaa', {
         paths: [
             path.join(__dirname, '/node_path/x'),
             path.join(__dirname, '/node_path/y')
         ],
-        basedir: __dirname
+        basedir: __dirname,
+        isDirectory: isDir
     }, function (err, res) {
         t.error(err);
         t.equal(res, path.join(__dirname, '/node_path/x/aaa/index.js'), 'aaa resolves');
@@ -21,7 +36,8 @@ test('$NODE_PATH', function (t) {
             path.join(__dirname, '/node_path/x'),
             path.join(__dirname, '/node_path/y')
         ],
-        basedir: __dirname
+        basedir: __dirname,
+        isDirectory: isDir
     }, function (err, res) {
         t.error(err);
         t.equal(res, path.join(__dirname, '/node_path/y/bbb/index.js'), 'bbb resolves');
@@ -32,21 +48,20 @@ test('$NODE_PATH', function (t) {
             path.join(__dirname, '/node_path/x'),
             path.join(__dirname, '/node_path/y')
         ],
-        basedir: __dirname
+        basedir: __dirname,
+        isDirectory: isDir
     }, function (err, res) {
         t.error(err);
         t.equal(res, path.join(__dirname, '/node_path/x/ccc/index.js'), 'ccc resolves');
     });
 
-    /*
-     * ensure that relative paths still resolve against the
-     * regular `node_modules` correctly
-     */
+    // ensure that relative paths still resolve against the regular `node_modules` correctly
     resolve('tap', {
         paths: [
             'node_path'
         ],
-        basedir: 'node_path/x'
+        basedir: path.join(__dirname, 'node_path/x'),
+        isDirectory: isDir
     }, function (err, res) {
         var root = require('tap/package.json').main;
         t.error(err);

test/resolver.js

@@ -364,15 +364,16 @@ test('async dot slash main', function (t) {
 });
 
 test('not a directory', function (t) {
-    t.plan(5);
+    t.plan(6);
     var path = './foo';
     resolve(path, { basedir: __filename }, function (err, res, pkg) {
         t.ok(err, 'a non-directory errors');
         t.equal(arguments.length, 1);
         t.equal(res, undefined);
         t.equal(pkg, undefined);
 
-        t.equal(err && err.message, 'Cannot find module \'' + path + "' from '" + __filename + "'");
+        t.equal(err && err.message, 'Cannot find module \'' + path + '\' from \'' + __filename + '\'');
+        t.equal(err && err.code, 'MODULE_NOT_FOUND');
     });
 });
 

test/resolver/multirepo/.npmrc

@@ -0,0 +1 @@
+package-lock=false

test/resolver/multirepo/lerna.json

@@ -0,0 +1,6 @@
+{
+  "packages": [
+    "packages/*"
+  ],
+  "version": "0.0.0"
+}

test/resolver/multirepo/package.json

@@ -0,0 +1,20 @@
+{
+  "name": "monorepo-symlink-test",
+  "private": true,
+  "version": "0.0.0",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    "postinstall": "lerna bootstrap",
+    "test": "node packages/package-a"
+  },
+  "author": "",
+  "license": "MIT",
+  "dependencies": {
+    "jquery": "^3.3.1",
+    "resolve": "../../../"
+  },
+  "devDependencies": {
+    "lerna": "^3.4.3"
+  }
+}

test/resolver/multirepo/packages/package-a/index.js

@@ -0,0 +1,35 @@
+'use strict';
+
+var assert = require('assert');
+var path = require('path');
+var resolve = require('resolve');
+
+var basedir = __dirname + '/node_modules/@my-scope/package-b';
+
+var expected = path.join(__dirname, '../../node_modules/jquery/dist/jquery.js');
+
+/*
+ * preserveSymlinks === false
+ * will search NPM package from
+ * - packages/package-b/node_modules
+ * - packages/node_modules
+ * - node_modules
+ */
+assert.equal(resolve.sync('jquery', { basedir: basedir, preserveSymlinks: false }), expected);
+assert.equal(resolve.sync('../../node_modules/jquery', { basedir: basedir, preserveSymlinks: false }), expected);
+
+/*
+ * preserveSymlinks === true
+ * will search NPM package from
+ * - packages/package-a/node_modules/@my-scope/packages/package-b/node_modules
+ * - packages/package-a/node_modules/@my-scope/packages/node_modules
+ * - packages/package-a/node_modules/@my-scope/node_modules
+ * - packages/package-a/node_modules/node_modules
+ * - packages/package-a/node_modules
+ * - packages/node_modules
+ * - node_modules
+ */
+assert.equal(resolve.sync('jquery', { basedir: basedir, preserveSymlinks: true }), expected);
+assert.equal(resolve.sync('../../../../../node_modules/jquery', { basedir: basedir, preserveSymlinks: true }), expected);
+
+console.log(' * all monorepo paths successfully resolved through symlinks');

test/resolver/multirepo/packages/package-a/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "@my-scope/package-a",
+  "version": "0.0.0",
+  "private": true,
+  "description": "",
+  "license": "MIT",
+  "main": "index.js",
+  "scripts": {
+    "test": "echo \"Error: run tests from root\" && exit 1"
+  },
+  "dependencies": {
+    "@my-scope/package-b": "^0.0.0"
+  }
+}

test/resolver/multirepo/packages/package-b/index.js

@@ -0,0 +1,14 @@
+{
+  "name": "@my-scope/package-b",
+  "private": true,
+  "version": "0.0.0",
+  "description": "",
+  "license": "MIT",
+  "main": "index.js",
+  "scripts": {
+    "test": "echo \"Error: run tests from root\" && exit 1"
+  },
+  "dependencies": {
+    "@my-scope/package-a": "^0.0.0"
+  }
+}