Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit ccbb463

Browse filesBrowse files
hefangshicjihrig
hefangshi
authored and
cjihrig
committed
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. Fixes: #6679 PR-URL: #6670 Reviewed-By: Evan Lucas <evanlucas@me.com>
1 parent 3f46b5c commit ccbb463
Copy full SHA for ccbb463

File tree

Expand file treeCollapse file tree

2 files changed

+119
-19
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+119
-19
lines changed
Open diff view settings
Collapse file

‎lib/module.js‎

Copy file name to clipboardExpand all lines: lib/module.js
+18-3Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,17 +221,29 @@ if (process.platform === 'win32') {
221221
// note: this approach *only* works when the path is guaranteed
222222
// to be absolute. Doing a fully-edge-case-correct path.split
223223
// that works on both Windows and Posix is non-trivial.
224+
225+
// return root node_modules when path is 'D:\\'.
226+
// path.resolve will make sure from.length >=3 in Windows.
227+
if (from.charCodeAt(from.length - 1) === 92/*\*/ &&
228+
from.charCodeAt(from.length - 2) === 58/*:*/)
229+
return [from + 'node_modules'];
230+
224231
const paths = [];
225232
var p = 0;
226233
var last = from.length;
227234
for (var i = from.length - 1; i >= 0; --i) {
228235
const code = from.charCodeAt(i);
229-
if (code === 92/*\*/ || code === 47/*/*/) {
236+
// The path segment separator check ('\' and '/') was used to get
237+
// node_modules path for every path segment.
238+
// Use colon as an extra condition since we can get node_modules
239+
// path for dirver root like 'C:\node_modules' and don't need to
240+
// parse driver name.
241+
if (code === 92/*\*/ || code === 47/*/*/ || code === 58/*:*/) {
230242
if (p !== nmLen)
231243
paths.push(from.slice(0, last) + '\\node_modules');
232244
last = i;
233245
p = 0;
234-
} else if (p !== -1 && p < nmLen) {
246+
} else if (p !== -1) {
235247
if (nmChars[p] === code) {
236248
++p;
237249
} else {
@@ -265,7 +277,7 @@ if (process.platform === 'win32') {
265277
paths.push(from.slice(0, last) + '/node_modules');
266278
last = i;
267279
p = 0;
268-
} else if (p !== -1 && p < nmLen) {
280+
} else if (p !== -1) {
269281
if (nmChars[p] === code) {
270282
++p;
271283
} else {
@@ -274,6 +286,9 @@ if (process.platform === 'win32') {
274286
}
275287
}
276288

289+
// Append /node_modules to handle root paths.
290+
paths.push('/node_modules');
291+
277292
return paths;
278293
};
279294
}
Collapse file

‎test/parallel/test-module-nodemodulepaths.js‎

Copy file name to clipboard
+101-16Lines changed: 101 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,105 @@
11
'use strict';
2-
var common = require('../common');
3-
var assert = require('assert');
42

5-
var module = require('module');
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const _module = require('module');
66

7-
var file, delimiter, paths;
7+
const cases = {
8+
'WIN': [{
9+
file: 'C:\\Users\\hefangshi\\AppData\\Roaming\
10+
\\npm\\node_modules\\npm\\node_modules\\minimatch',
11+
expect: [
12+
'C:\\Users\\hefangshi\\AppData\\Roaming\
13+
\\npm\\node_modules\\npm\\node_modules\\minimatch\\node_modules',
14+
'C:\\Users\\hefangshi\\AppData\\Roaming\
15+
\\npm\\node_modules\\npm\\node_modules',
16+
'C:\\Users\\hefangshi\\AppData\\Roaming\\npm\\node_modules',
17+
'C:\\Users\\hefangshi\\AppData\\Roaming\\node_modules',
18+
'C:\\Users\\hefangshi\\AppData\\node_modules',
19+
'C:\\Users\\hefangshi\\node_modules',
20+
'C:\\Users\\node_modules',
21+
'C:\\node_modules'
22+
]
23+
}, {
24+
file: 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo',
25+
expect: [
26+
'C:\\Users\\Rocko Artischocko\\node_stuff\\foo\\node_modules',
27+
'C:\\Users\\Rocko Artischocko\\node_stuff\\node_modules',
28+
'C:\\Users\\Rocko Artischocko\\node_modules',
29+
'C:\\Users\\node_modules',
30+
'C:\\node_modules'
31+
]
32+
}, {
33+
file: 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo_node_modules',
34+
expect: [
35+
'C:\\Users\\Rocko \
36+
Artischocko\\node_stuff\\foo_node_modules\\node_modules',
37+
'C:\\Users\\Rocko Artischocko\\node_stuff\\node_modules',
38+
'C:\\Users\\Rocko Artischocko\\node_modules',
39+
'C:\\Users\\node_modules',
40+
'C:\\node_modules'
41+
]
42+
}, {
43+
file: 'C:\\node_modules',
44+
expect: [
45+
'C:\\node_modules'
46+
]
47+
}, {
48+
file: 'C:\\',
49+
expect: [
50+
'C:\\node_modules'
51+
]
52+
}],
53+
'POSIX': [{
54+
file: '/usr/lib/node_modules/npm/node_modules/\
55+
node-gyp/node_modules/glob/node_modules/minimatch',
56+
expect: [
57+
'/usr/lib/node_modules/npm/node_modules/\
58+
node-gyp/node_modules/glob/node_modules/minimatch/node_modules',
59+
'/usr/lib/node_modules/npm/node_modules/\
60+
node-gyp/node_modules/glob/node_modules',
61+
'/usr/lib/node_modules/npm/node_modules/node-gyp/node_modules',
62+
'/usr/lib/node_modules/npm/node_modules',
63+
'/usr/lib/node_modules',
64+
'/usr/node_modules',
65+
'/node_modules'
66+
]
67+
}, {
68+
file: '/usr/test/lib/node_modules/npm/foo',
69+
expect: [
70+
'/usr/test/lib/node_modules/npm/foo/node_modules',
71+
'/usr/test/lib/node_modules/npm/node_modules',
72+
'/usr/test/lib/node_modules',
73+
'/usr/test/node_modules',
74+
'/usr/node_modules',
75+
'/node_modules'
76+
]
77+
}, {
78+
file: '/usr/test/lib/node_modules/npm/foo_node_modules',
79+
expect: [
80+
'/usr/test/lib/node_modules/npm/foo_node_modules/node_modules',
81+
'/usr/test/lib/node_modules/npm/node_modules',
82+
'/usr/test/lib/node_modules',
83+
'/usr/test/node_modules',
84+
'/usr/node_modules',
85+
'/node_modules'
86+
]
87+
}, {
88+
file: '/node_modules',
89+
expect: [
90+
'/node_modules'
91+
]
92+
}, {
93+
file: '/',
94+
expect: [
95+
'/node_modules'
96+
]
97+
}]
98+
};
899

9-
if (common.isWindows) {
10-
file = 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo';
11-
delimiter = '\\';
12-
} else {
13-
file = '/usr/test/lib/node_modules/npm/foo';
14-
delimiter = '/';
15-
}
16-
17-
paths = module._nodeModulePaths(file);
18-
19-
assert.ok(paths.indexOf(file + delimiter + 'node_modules') !== -1);
20-
assert.ok(Array.isArray(paths));
100+
const platformCases = common.isWindows ? cases.WIN : cases.POSIX;
101+
platformCases.forEach((c) => {
102+
const paths = _module._nodeModulePaths(c.file);
103+
assert.deepStrictEqual(c.expect, paths, 'case ' + c.file +
104+
' failed, actual paths is ' + JSON.stringify(paths));
105+
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.