Skip to content

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

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.

Sign up for GitHub

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

Closed
hefangshi wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
126cbfd
module: fix node_modules search path in edge case
hefangshi May 10, 2016
145f276
module: fix root path node_modules missing issue
hefangshi May 11, 2016
ff893e4
test: add real world case for node_modules paths
hefangshi Jul 11, 2016
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
hefangshi committed Aug 2, 2016
commit ff893e4ba7d76dbfc1ee4265488dc2b06a6bb31d
10 changes: 7 additions & 3 deletions lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;
Expand Down
29 changes: 29 additions & 0 deletions test/parallel/test-module-nodemodulepaths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down