module: fix node_modules search path in edge case

hefangshi · cjihrig · commit ccbb46378ff9 · 2016-08-10T12:56:52.000-04:00
The `p < nmLen` condition will fail when a module's name is end with `node_modules` like `foo_node_modules`. The old logic will miss the `foo_node_modules/node_modules` in node_modules paths. TL;TR, a module named like `foo_node_modules` can't require any module in the node_modules folder. Fixes: #6679 PR-URL: #6670 Reviewed-By: Evan Lucas <evanlucas@me.com>
diff --git a/lib/module.js b/lib/module.js
@@ -221,17 +221,29 @@ if (process.platform === 'win32') {
     // note: this approach *only* works when the path is guaranteed
     // to be absolute.  Doing a fully-edge-case-correct path.split
     // that works on both Windows and Posix is non-trivial.
+
+    // return root node_modules when path is 'D:\\'.
+    // path.resolve will make sure from.length >=3 in Windows.
+    if (from.charCodeAt(from.length - 1) === 92/*\*/ &&
+        from.charCodeAt(from.length - 2) === 58/*:*/)
+      return [from + 'node_modules'];
+
     const paths = [];
     var p = 0;
     var last = from.length;
     for (var i = from.length - 1; i >= 0; --i) {
       const code = from.charCodeAt(i);
-      if (code === 92/*\*/ || code === 47/*/*/) {
+      // The path segment separator check ('\' and '/') was used to get
+      // node_modules path for every path segment.
+      // Use colon as an extra condition since we can get node_modules
+      // path for dirver root like 'C:\node_modules' and don't need to
+      // parse driver name.
+      if (code === 92/*\*/ || code === 47/*/*/ || code === 58/*:*/) {
         if (p !== nmLen)
           paths.push(from.slice(0, last) + '\\node_modules');
         last = i;
         p = 0;
-      } else if (p !== -1 && p < nmLen) {
+      } else if (p !== -1) {
         if (nmChars[p] === code) {
           ++p;
         } else {
@@ -265,7 +277,7 @@ if (process.platform === 'win32') {
           paths.push(from.slice(0, last) + '/node_modules');
         last = i;
         p = 0;
-      } else if (p !== -1 && p < nmLen) {
+      } else if (p !== -1) {
         if (nmChars[p] === code) {
           ++p;
         } else {
@@ -274,6 +286,9 @@ if (process.platform === 'win32') {
       }
     }
 
+    // Append /node_modules to handle root paths.
+    paths.push('/node_modules');
+
     return paths;
   };
 }
diff --git a/test/parallel/test-module-nodemodulepaths.js b/test/parallel/test-module-nodemodulepaths.js
@@ -1,20 +1,105 @@
 'use strict';
-var common = require('../common');
-var assert = require('assert');
 
-var module = require('module');
+const common = require('../common');
+const assert = require('assert');
+const _module = require('module');
 
-var file, delimiter, paths;
+const cases = {
+  'WIN': [{
+    file: 'C:\\Users\\hefangshi\\AppData\\Roaming\
+\\npm\\node_modules\\npm\\node_modules\\minimatch',
+    expect: [
+      'C:\\Users\\hefangshi\\AppData\\Roaming\
+\\npm\\node_modules\\npm\\node_modules\\minimatch\\node_modules',
+      'C:\\Users\\hefangshi\\AppData\\Roaming\
+\\npm\\node_modules\\npm\\node_modules',
+      'C:\\Users\\hefangshi\\AppData\\Roaming\\npm\\node_modules',
+      'C:\\Users\\hefangshi\\AppData\\Roaming\\node_modules',
+      'C:\\Users\\hefangshi\\AppData\\node_modules',
+      'C:\\Users\\hefangshi\\node_modules',
+      'C:\\Users\\node_modules',
+      'C:\\node_modules'
+    ]
+  }, {
+    file: 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo',
+    expect: [
+      'C:\\Users\\Rocko Artischocko\\node_stuff\\foo\\node_modules',
+      'C:\\Users\\Rocko Artischocko\\node_stuff\\node_modules',
+      'C:\\Users\\Rocko Artischocko\\node_modules',
+      'C:\\Users\\node_modules',
+      'C:\\node_modules'
+    ]
+  }, {
+    file: 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo_node_modules',
+    expect: [
+      'C:\\Users\\Rocko \
+Artischocko\\node_stuff\\foo_node_modules\\node_modules',
+      'C:\\Users\\Rocko Artischocko\\node_stuff\\node_modules',
+      'C:\\Users\\Rocko Artischocko\\node_modules',
+      'C:\\Users\\node_modules',
+      'C:\\node_modules'
+    ]
+  }, {
+    file: 'C:\\node_modules',
+    expect: [
+      'C:\\node_modules'
+    ]
+  }, {
+    file: 'C:\\',
+    expect: [
+      'C:\\node_modules'
+    ]
+  }],
+  'POSIX': [{
+    file: '/usr/lib/node_modules/npm/node_modules/\
+node-gyp/node_modules/glob/node_modules/minimatch',
+    expect: [
+      '/usr/lib/node_modules/npm/node_modules/\
+node-gyp/node_modules/glob/node_modules/minimatch/node_modules',
+      '/usr/lib/node_modules/npm/node_modules/\
+node-gyp/node_modules/glob/node_modules',
+      '/usr/lib/node_modules/npm/node_modules/node-gyp/node_modules',
+      '/usr/lib/node_modules/npm/node_modules',
+      '/usr/lib/node_modules',
+      '/usr/node_modules',
+      '/node_modules'
+    ]
+  }, {
+    file: '/usr/test/lib/node_modules/npm/foo',
+    expect: [
+      '/usr/test/lib/node_modules/npm/foo/node_modules',
+      '/usr/test/lib/node_modules/npm/node_modules',
+      '/usr/test/lib/node_modules',
+      '/usr/test/node_modules',
+      '/usr/node_modules',
+      '/node_modules'
+    ]
+  }, {
+    file: '/usr/test/lib/node_modules/npm/foo_node_modules',
+    expect: [
+      '/usr/test/lib/node_modules/npm/foo_node_modules/node_modules',
+      '/usr/test/lib/node_modules/npm/node_modules',
+      '/usr/test/lib/node_modules',
+      '/usr/test/node_modules',
+      '/usr/node_modules',
+      '/node_modules'
+    ]
+  }, {
+    file: '/node_modules',
+    expect: [
+      '/node_modules'
+    ]
+  }, {
+    file: '/',
+    expect: [
+      '/node_modules'
+    ]
+  }]
+};
 
-if (common.isWindows) {
-  file = 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo';
-  delimiter = '\\';
-} else {
-  file = '/usr/test/lib/node_modules/npm/foo';
-  delimiter = '/';
-}
-
-paths = module._nodeModulePaths(file);
-
-assert.ok(paths.indexOf(file + delimiter + 'node_modules') !== -1);
-assert.ok(Array.isArray(paths));
+const platformCases = common.isWindows ? cases.WIN : cases.POSIX;
+platformCases.forEach((c) => {
+  const paths = _module._nodeModulePaths(c.file);
+  assert.deepStrictEqual(c.expect, paths, 'case ' + c.file +
+    ' failed, actual paths is ' + JSON.stringify(paths));
+});
