Skip to content

Implements useProcessResolution #170

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
wants to merge 7 commits into from
Closed

Implements useProcessResolution #170

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e011edc
Implements paths=function and useNodeModules=false
arcanis Oct 19, 2018
5f11711
Makes nodeModulesPaths accept the request
arcanis Oct 19, 2018
109f275
Implements useProcessResolution
arcanis Oct 19, 2018
5797755
Adds documentation
arcanis Oct 19, 2018
5655d0a
Merge branch 'master' into pnp-compat
arcanis Oct 25, 2018
c9faab9
Makes the request an option rather than a parameter
Oct 26, 2018
9daf6b4
Renames useNodeModules into skipNodeModules
Oct 26, 2018
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
7 changes: 6 additions & 1 deletion lib/async.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ var fs = require('fs');
var path = require('path');
var caller = require('./caller.js');
var nodeModulesPaths = require('./node-modules-paths.js');
var useProcessResolution = require('./use-process-resolution.js');

var defaultIsFile = function isFile(file, cb) {
fs.stat(file, function (err, stat) {
Expand Down Expand Up @@ -38,6 +39,10 @@ module.exports = function resolve(x, options, callback) {
});
}

if (opts.useProcessResolution) {
useProcessResolution(opts);
}

var isFile = opts.isFile || defaultIsFile;
var isDirectory = opts.isDirectory || defaultIsDir;
var readFile = opts.readFile || fs.readFile;
Expand Down Expand Up @@ -225,6 +230,6 @@ module.exports = function resolve(x, options, callback) {
}
}
function loadNodeModules(x, start, cb) {
processDirs(cb, nodeModulesPaths(start, opts));
processDirs(cb, nodeModulesPaths(x, start, opts));
}
};
48 changes: 30 additions & 18 deletions lib/node-modules-paths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ var path = require('path');
var fs = require('fs');
var parse = path.parse || require('path-parse');

module.exports = function nodeModulesPaths(start, opts) {
module.exports = function nodeModulesPaths(x, start, opts) {
Copy link
Member

@ljharb ljharb Oct 26, 2018

Choose a reason for hiding this comment

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

changing the signature in this way makes it a breaking change; also, what is "x"?

can we make this be the last argument instead?

Copy link
Contributor Author

@arcanis arcanis Oct 26, 2018

Choose a reason for hiding this comment

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

I wasn't sure if it was a publicly exposed function, since it wasn't listed in the doc.

x is the request being made (took the variable name from other parts of the code). It can easily be moved to the end of the argument list, for sure

Copy link
Member

@ljharb ljharb Oct 29, 2018

Choose a reason for hiding this comment

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

Everything reachable is part of the public API; docs are irrelevant - so any break in anything that's exposed would be semver-major.

var modules = opts && opts.moduleDirectory
? [].concat(opts.moduleDirectory)
: ['node_modules'];
Expand All @@ -20,25 +20,37 @@ module.exports = function nodeModulesPaths(start, opts) {
}
}

var prefix = '/';
if ((/^([A-Za-z]:)/).test(absoluteStart)) {
prefix = '';
} else if ((/^\\\\/).test(absoluteStart)) {
prefix = '\\\\';
}
var dirs = [];

if (!opts || typeof opts.useNodeModules === 'undefined' || opts.useNodeModules) {
Copy link
Member

@ljharb ljharb Oct 26, 2018

Choose a reason for hiding this comment

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

boolean options should default to false, so that absence and false are the same - let's invert the naming of this option.

it'd also be nice to minimize the git diffs if possible.

Copy link
Contributor Author

@arcanis arcanis Oct 26, 2018

Choose a reason for hiding this comment

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

Renamed the option - minimizing the diff would involve doing unnecessary operations at runtime (or breaking the indentation), I'd like to avoid that :/

Copy link
Member

@ljharb ljharb Oct 29, 2018

Choose a reason for hiding this comment

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

it seems like we could maybe use early returns here, or extract pieces into helper functions?

Copy link
Contributor Author

@arcanis arcanis Nov 2, 2018

Choose a reason for hiding this comment

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

I'm not sure it would drastically improve the code, but if you feel strongly I can do it. What I like with the current formatting is that the order in which the transformations are applied is quite clear - a random user can easily follow it from beginning to end. Early return could make this slightly harder, what do you think?

var prefix = '/';
if ((/^([A-Za-z]:)/).test(absoluteStart)) {
prefix = '';
} else if ((/^\\\\/).test(absoluteStart)) {
prefix = '\\\\';
}

var paths = [absoluteStart];
var parsed = parse(absoluteStart);
while (parsed.dir !== paths[paths.length - 1]) {
paths.push(parsed.dir);
parsed = parse(parsed.dir);
var paths = [absoluteStart];
var parsed = parse(absoluteStart);
while (parsed.dir !== paths[paths.length - 1]) {
paths.push(parsed.dir);
parsed = parse(parsed.dir);
}

dirs = paths.reduce(function (dirs, aPath) {
return dirs.concat(modules.map(function (moduleDir) {
return path.join(prefix, aPath, moduleDir);
}));
}, []);
}

var dirs = paths.reduce(function (dirs, aPath) {
return dirs.concat(modules.map(function (moduleDir) {
return path.join(prefix, aPath, moduleDir);
}));
}, []);
if (opts && opts.paths) {
if (typeof opts.paths === 'function') {
dirs = dirs.concat(opts.paths(x, absoluteStart));
} else {
dirs = dirs.concat(opts.paths);
}
}

return opts && opts.paths ? dirs.concat(opts.paths) : dirs;
return dirs;
};
8 changes: 7 additions & 1 deletion lib/sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ var fs = require('fs');
var path = require('path');
var caller = require('./caller.js');
var nodeModulesPaths = require('./node-modules-paths.js');
var useProcessResolution = require('./use-process-resolution.js');

var defaultIsFile = function isFile(file) {
try {
Expand All @@ -29,6 +30,11 @@ module.exports = function (x, options) {
throw new TypeError('Path must be a string.');
}
var opts = options || {};

if (opts.useProcessResolution) {
useProcessResolution(opts);
}

var isFile = opts.isFile || defaultIsFile;
var isDirectory = opts.isDirectory || defaultIsDir;
var readFileSync = opts.readFileSync || fs.readFileSync;
Expand Down Expand Up @@ -137,7 +143,7 @@ module.exports = function (x, options) {
}

function loadNodeModulesSync(x, start) {
var dirs = nodeModulesPaths(start, opts);
var dirs = nodeModulesPaths(x, start, opts);
for (var i = 0; i < dirs.length; i++) {
var dir = dirs[i];
var m = loadAsFileSync(path.join(dir, '/', x));
Expand Down
27 changes: 27 additions & 0 deletions lib/use-process-resolution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
var path = require('path');

module.exports = function processResolution(opts) {
if (process.versions.pnp) {
var pnp = require('pnpapi');

opts.preserveSymlinks = true;
opts.useNodeModules = false;
opts.paths = function (request, basedir) {
// Extract the name of the package being requested (1=full name, 2=scope name, 3=local name)
var parts = request.match(/^((?:(@[^\/]+)\/)?([^\/]+))/);

// This is guaranteed to return the path to the "package.json" file from the given package
var manifestPath = pnp.resolveToUnqualified(parts[1] + '/package.json', basedir);

// The first dirname strips the package.json, the second strips the local named folder
var nodeModules = path.dirname(path.dirname(manifestPath));

// Strips the scope named folder if needed
if (parts[2]) {
nodeModules = path.dirname(nodeModules);
}

return [nodeModules];
};
}
};
4 changes: 4 additions & 0 deletions readme.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ options are:

* opts.paths - require.paths array to use if nothing is found on the normal `node_modules` recursive walk (probably don't use this)

* opts.useProcessResolution - instructs `resolve` to use the same resolution algorithm than the one used by the current process

* opts.useNodeModules - instructs `resolve` to automatically locate the `node_modules` directories. default: true

* opts.moduleDirectory - directory (or directories) in which to recursively look for modules. default: `"node_modules"`

* opts.preserveSymlinks - if true, doesn't resolve `basedir` to real path before resolving.
Expand Down
45 changes: 37 additions & 8 deletions test/node-modules-paths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ var nodeModulesPaths = require('../lib/node-modules-paths');

var verifyDirs = function verifyDirs(t, start, dirs, moduleDirectories, paths) {
var moduleDirs = [].concat(moduleDirectories || 'node_modules');
if (paths) {
for (var k = 0; k < paths.length; ++k) {
moduleDirs.push(path.basename(paths[k]));
Copy link
Member

@ljharb ljharb Oct 26, 2018

Choose a reason for hiding this comment

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

this seems like a behavior change - the test is now using path.basename on all the paths instead of passing them in directly.

Copy link
Contributor Author

@arcanis arcanis Oct 26, 2018

Choose a reason for hiding this comment

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

Only one test was using the path parameter previously and it was providing straight folder names (['a', 'b']), so it shouldn't change what's being tested

Copy link
Member

@ljharb ljharb Oct 29, 2018

Choose a reason for hiding this comment

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

Fair, but why is the basename change needed? Was it incorrect for the test to omit it previously (and there weren't enough test cases to demonstrate that)?

Copy link
Contributor Author

@arcanis arcanis Nov 2, 2018

Choose a reason for hiding this comment

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

Oops, missed the comment, my bad! The basename change was needed because the previous testcases were only using paths to contain single-component paths. Now that there's a test that adds an absolute path, we must be careful not to add the full absolute path into the moduleDirs variable.

}
}

var foundModuleDirs = {};
var uniqueDirs = {};
Expand All @@ -20,7 +25,7 @@ var verifyDirs = function verifyDirs(t, start, dirs, moduleDirectories, paths) {
}
t.equal(keys(parsedDirs).length >= start.split(path.sep).length, true, 'there are >= dirs than "start" has');
var foundModuleDirNames = keys(foundModuleDirs);
t.deepEqual(foundModuleDirNames, moduleDirs.concat(paths || []), 'all desired module dirs were found');
t.deepEqual(foundModuleDirNames, moduleDirs, 'all desired module dirs were found');
t.equal(keys(uniqueDirs).length, dirs.length, 'all dirs provided were unique');

var counts = {};
Expand All @@ -33,7 +38,7 @@ var verifyDirs = function verifyDirs(t, start, dirs, moduleDirectories, paths) {
test('node-modules-paths', function (t) {
t.test('no options', function (t) {
var start = path.join(__dirname, 'resolver');
var dirs = nodeModulesPaths(start);
var dirs = nodeModulesPaths('pkg', start);

verifyDirs(t, start, dirs);

Expand All @@ -42,27 +47,51 @@ test('node-modules-paths', function (t) {

t.test('empty options', function (t) {
var start = path.join(__dirname, 'resolver');
var dirs = nodeModulesPaths(start, {});
var dirs = nodeModulesPaths('pkg', start, {});

verifyDirs(t, start, dirs);

t.end();
});

t.test('with paths option', function (t) {
t.test('with useNodeModules=false option', function (t) {
var start = path.join(__dirname, 'resolver');
var dirs = nodeModulesPaths('pkg', start, { useNodeModules: false });

t.deepEqual(dirs, [], 'no node_modules was computed');

t.end();
});

t.test('with paths=array option', function (t) {
var start = path.join(__dirname, 'resolver');
var paths = ['a', 'b'];
var dirs = nodeModulesPaths(start, { paths: paths });
var dirs = nodeModulesPaths('pkg', start, { paths: paths });

verifyDirs(t, start, dirs, null, paths);

t.end();
});

t.test('with paths=function option', function (t) {
var paths = function paths(absoluteStart) {
return [path.join(absoluteStart, 'not node modules')];
};

var start = path.join(__dirname, 'resolver');
var dirs = nodeModulesPaths('pkg', start, { paths: paths });

console.log(dirs);

verifyDirs(t, start, dirs, null, [path.join(start, 'not node modules')]);

t.end();
});

t.test('with moduleDirectory option', function (t) {
var start = path.join(__dirname, 'resolver');
var moduleDirectory = 'not node modules';
var dirs = nodeModulesPaths(start, { moduleDirectory: moduleDirectory });
var dirs = nodeModulesPaths('pkg', start, { moduleDirectory: moduleDirectory });

verifyDirs(t, start, dirs, moduleDirectory);

Expand All @@ -73,7 +102,7 @@ test('node-modules-paths', function (t) {
var start = path.join(__dirname, 'resolver');
var paths = ['a', 'b'];
var moduleDirectory = 'not node modules';
var dirs = nodeModulesPaths(start, { paths: paths, moduleDirectory: moduleDirectory });
var dirs = nodeModulesPaths('pkg', start, { paths: paths, moduleDirectory: moduleDirectory });

verifyDirs(t, start, dirs, moduleDirectory, paths);

Expand All @@ -84,7 +113,7 @@ test('node-modules-paths', function (t) {
var start = path.join(__dirname, 'resolver');
var paths = ['a', 'b'];
var moduleDirectories = ['not node modules', 'other modules'];
var dirs = nodeModulesPaths(start, { paths: paths, moduleDirectory: moduleDirectories });
var dirs = nodeModulesPaths('pkg', start, { paths: paths, moduleDirectory: moduleDirectories });

verifyDirs(t, start, dirs, moduleDirectories, paths);

Expand Down