Skip to content

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

Rodrigo Muino Tomonari requested to merge github/fork/hefangshi/fix-node_modules-paths into master
Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

module

Description of change

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 his node_modules folder.

Here is a demo which compared the new nodeModulePaths with the old one.

const path = require('path');
var nmChars = [115, 101, 108, 117, 100, 111, 109, 95, 101, 100, 111, 110];
var nmLen = nmChars.length;

function nodeModulePaths(from) {
    // guarantee that 'from' is absolute.
    from = path.resolve(from);
    // Return early not only to avoid unnecessary work, but to *avoid* returning
    // an array of two items for a root: [ '//node_modules', '/node_modules' ]
    if (from === '/')
        return ['/node_modules'];

    // 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.
    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 === 47 /*/*/ ) {
            if (p !== nmLen)
                paths.push(from.slice(0, last) + '/node_modules');
            last = i;
            p = 0;
        }
        else if (p !== -1 && p < nmLen) {
            if (nmChars[p] === code) {
                ++p;
            }
            else {
                p = -1;
            }
        }
    }

    return paths;
};

const splitRe = process.platform === 'win32' ? /[\/\\]/ : /\//;
nodeModulePathsOld = function (from) {
    // guarantee that 'from' is absolute.
    from = path.resolve(from);
    // 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.
    var paths = [];
    var parts = from.split(splitRe);
    for (var tip = parts.length - 1; tip >= 0; tip--) {
        // don't search in .../node_modules/node_modules
        if (parts[tip] === 'node_modules') continue;
        var dir = parts.slice(0, tip + 1).concat('node_modules').join(path.sep);
        paths.push(dir);
    }
    return paths;
}

console.log('new:', nodeModulePaths('/Users/hefangshi/test/foo_node_modules'));
console.log('old:', nodeModulePathsOld('/Users/hefangshi/test/foo_node_modules'));

Output:

new: [ '/Users/hefangshi/test/node_modules',
  '/Users/hefangshi/node_modules',
  '/Users/node_modules' ]
old: [ '/Users/hefangshi/test/foo_node_modules/node_modules',
  '/Users/hefangshi/test/node_modules',
  '/Users/hefangshi/node_modules',
  '/Users/node_modules',
  '/node_modules' ]

The main reason is the p < nmLen condition will ignore foo_node_modules and simply think it should be /node_modules, and won't search /Users/hefangshi/test/foo_node_modules/node_modules for dependency.

I already tested in mac and windows, but the testcases were not very full edge tested.

@mscdex please have a look on it.

Merge request reports