From 126cbfd4c18dbd997460adfca060257766fae308 Mon Sep 17 00:00:00 2001 From: hefangshi Date: Tue, 10 May 2016 16:16:49 +0800 Subject: [PATCH 1/3] module: fix node_modules search path in edge case 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. --- lib/module.js | 4 +- test/parallel/test-module-nodemodulepaths.js | 65 +++++++++++++++----- 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/lib/module.js b/lib/module.js index 344ea97af91953..292c369c2018e2 100644 --- a/lib/module.js +++ b/lib/module.js @@ -229,7 +229,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 { @@ -263,7 +263,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 { diff --git a/test/parallel/test-module-nodemodulepaths.js b/test/parallel/test-module-nodemodulepaths.js index 0c70de9f285c3c..fc9ad271dbb66e 100644 --- a/test/parallel/test-module-nodemodulepaths.js +++ b/test/parallel/test-module-nodemodulepaths.js @@ -1,20 +1,53 @@ '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\\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', + ] + }], + 'UNIX': [{ + 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', + ] + }, { + 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', + ] + }] +}; -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.UNIX; +platformCases.forEach((c) => { + const paths = _module._nodeModulePaths(c.file); + assert.deepStrictEqual(c.expect, paths); +}); From 145f276cd0b93fc721228c5bec013127688dadf6 Mon Sep 17 00:00:00 2001 From: hefangshi Date: Thu, 12 May 2016 01:20:15 +0800 Subject: [PATCH 2/3] module: fix root path node_modules missing issue Manual parsers didn't handle the root path on both platform, so push driver root node_modules when colon is matched in windows to avoid parse dirver name and direct push `/node_modules` into paths in posix. --- lib/module.js | 13 +++++++- test/parallel/test-module-nodemodulepaths.js | 33 +++++++++++++++++--- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/lib/module.js b/lib/module.js index 292c369c2018e2..9aabad0fd3acb5 100644 --- a/lib/module.js +++ b/lib/module.js @@ -219,12 +219,21 @@ 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/*/*/) { + // use colon as a split to add driver root node_modules + // it's a little more tricky than posix version to avoid parse drive name. + if (code === 92/*\*/ || code === 47/*/*/ || code === 58/*:*/) { if (p !== nmLen) paths.push(from.slice(0, last) + '\\node_modules'); last = i; @@ -271,6 +280,8 @@ if (process.platform === 'win32') { } } } + // append /node_modules since this approach didn't handle root path + paths.push('/node_modules'); return paths; }; diff --git a/test/parallel/test-module-nodemodulepaths.js b/test/parallel/test-module-nodemodulepaths.js index fc9ad271dbb66e..969d481387a51e 100644 --- a/test/parallel/test-module-nodemodulepaths.js +++ b/test/parallel/test-module-nodemodulepaths.js @@ -12,7 +12,7 @@ const cases = { 'C:\\Users\\Rocko Artischocko\\node_stuff\\node_modules', 'C:\\Users\\Rocko Artischocko\\node_modules', 'C:\\Users\\node_modules', - 'C:\\node_modules', + 'C:\\node_modules' ] }, { file: 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo_node_modules', @@ -22,10 +22,20 @@ 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', + 'C:\\node_modules' + ] + }, { + file: 'C:\\node_modules', + expect: [ + 'C:\\node_modules' + ] + }, { + file: 'C:\\', + expect: [ + 'C:\\node_modules' ] }], - 'UNIX': [{ + 'POSIX': [{ file: '/usr/test/lib/node_modules/npm/foo', expect: [ '/usr/test/lib/node_modules/npm/foo/node_modules', @@ -33,6 +43,7 @@ Artischocko\\node_stuff\\foo_node_modules\\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', @@ -42,12 +53,24 @@ Artischocko\\node_stuff\\foo_node_modules\\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' ] }] }; -const platformCases = common.isWindows ? cases.WIN : cases.UNIX; +const platformCases = common.isWindows ? cases.WIN : cases.POSIX; platformCases.forEach((c) => { const paths = _module._nodeModulePaths(c.file); - assert.deepStrictEqual(c.expect, paths); + assert.deepStrictEqual(c.expect, paths, 'case ' + c.file + + ' failed, actual paths is ' + JSON.stringify(paths)); }); From ff893e4ba7d76dbfc1ee4265488dc2b06a6bb31d Mon Sep 17 00:00:00 2001 From: hefangshi Date: Mon, 11 Jul 2016 14:17:01 +0800 Subject: [PATCH 3/3] test: add real world case for node_modules paths Add a real world global node_modules path test case come from npm's dependency to make test more effective. --- lib/module.js | 10 +++++-- test/parallel/test-module-nodemodulepaths.js | 29 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/lib/module.js b/lib/module.js index 9aabad0fd3acb5..c20a9cef775dcb 100644 --- a/lib/module.js +++ b/lib/module.js @@ -231,8 +231,11 @@ if (process.platform === 'win32') { var last = from.length; for (var i = from.length - 1; i >= 0; --i) { const code = from.charCodeAt(i); - // use colon as a split to add driver root node_modules - // it's a little more tricky than posix version to avoid parse drive name. + // 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'); @@ -280,7 +283,8 @@ if (process.platform === 'win32') { } } } - // append /node_modules since this approach didn't handle root path + + // 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 index 969d481387a51e..02ea79b49e1ab3 100644 --- a/test/parallel/test-module-nodemodulepaths.js +++ b/test/parallel/test-module-nodemodulepaths.js @@ -6,6 +6,21 @@ const _module = require('module'); 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', @@ -36,6 +51,20 @@ Artischocko\\node_stuff\\foo_node_modules\\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',