Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: fix wrong condition in early return logic for node_module path #6670

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
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.
  • Loading branch information
hefangshi committed Aug 2, 2016
commit ff893e4ba7d76dbfc1ee4265488dc2b06a6bb31d
10 changes: 7 additions & 3 deletions lib/module.js
Original file line number Diff line number Diff line change
@@ -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;
29 changes: 29 additions & 0 deletions test/parallel/test-module-nodemodulepaths.js
Original file line number Diff line number Diff line change
@@ -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',