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
Next Next commit
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.
  • Loading branch information
@hefangshi
hefangshi committed Aug 2, 2016
commit 145f276cd0b93fc721228c5bec013127688dadf6
13 changes: 12 additions & 1 deletion lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig Aug 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments seem to only apply to the colon, added in this PR. Anyone stumbling on this in the future might be confused by it. Could you expand the comment please. Also, please start comments with a capital letter, and terminate with a period.

Copy link
Copy Markdown
Contributor Author

@hefangshi hefangshi Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, feel free to give me some advise about the comments since I don't know English too well.

// 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;
Expand Down Expand Up @@ -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;
};
Expand Down
33 changes: 28 additions & 5 deletions test/parallel/test-module-nodemodulepaths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -22,17 +22,28 @@ 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',
'/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',
Expand All @@ -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));
});