Skip to content

Implements packageIterator #205

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

Merged
merged 1 commit into from
Jan 22, 2020
Merged

Implements packageIterator #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 17 additions & 5 deletions lib/async.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ var maybeUnwrapSymlink = function maybeUnwrapSymlink(x, opts, cb) {
}
};

var getPackageCandidates = function getPackageCandidates(x, start, opts) {
var dirs = nodeModulesPaths(start, opts, x);
for (var i = 0; i < dirs.length; i++) {
dirs[i] = path.join(dirs[i], x);
}
return dirs;
};

module.exports = function resolve(x, options, callback) {
var cb = callback;
var opts = options;
Expand All @@ -55,6 +63,7 @@ module.exports = function resolve(x, options, callback) {
var isFile = opts.isFile || defaultIsFile;
var isDirectory = opts.isDirectory || defaultIsDir;
var readFile = opts.readFile || fs.readFile;
var packageIterator = opts.packageIterator;

var extensions = opts.extensions || ['.js'];
var basedir = opts.basedir || path.dirname(caller());
Expand Down Expand Up @@ -265,19 +274,18 @@ module.exports = function resolve(x, options, callback) {
if (dirs.length === 0) return cb(null, undefined);
var dir = dirs[0];

isDirectory(dir, isdir);
isDirectory(path.dirname(dir), isdir);
Copy link
Member

@ljharb ljharb Jan 7, 2020

Choose a reason for hiding this comment

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

when is the dirname not going to be a directory? the intention here, i believe, is to see if dir points to a file or not. It seems like hoisting the path.join prevents that from being determined.

Copy link
Contributor Author

@arcanis arcanis Jan 7, 2020

Choose a reason for hiding this comment

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

The dirname isn't a directory when it doesn't exist. From what I gather from #190 and #116, this code was only meant to speed up the resolution by checking whether the n_m folder existed before doing the file stat calls.

Checking whether the parent of the subpath (ie /n_m/foo for foo/hello) exists has the same behaviours: if foo/hello can be resolved, then /n_m/foo will necessarily be a directory.

Copy link
Contributor Author

@arcanis arcanis Jan 7, 2020

Choose a reason for hiding this comment

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

In other words:

request partial resolution isDirectory (before) isDirectory (after)
foo /n_m/foo /n_m /n_m
foo/bar /n_m/foo/bar /n_m /n_m/foo
@foo/bar /n_m/@foo/bar /n_m /n_m/@foo

The effect will be the same before and after: the two stat calls that the code would have made before #190 (taking the first line as an example, one to check whether /n_m/foo.js exists and another to check whether /n_m/foo/index.js exists) won't be necessary because resolve is able to find out that the directory supposed to contain them doesn't even exist.

Copy link
Member

@ljharb ljharb Jan 7, 2020

Choose a reason for hiding this comment

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

I see. Do you think that this perf improvement could also be split out into a separate PR (or even just a separate commit prior to your addition)?

Copy link
Contributor Author

@arcanis arcanis Jan 7, 2020

Choose a reason for hiding this comment

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

In this PR it's not a perf improvement, it's necessary to make my PR work with the changes brought by #190 (because with my PR we don't have access to n_m paths anymore in loadNodeModules - instead we only have /n_m/<subpath>). I guess I could revert it and re-apply it, but I'm not sure it would be much clearer 🤔

Copy link
Member

@ljharb ljharb Jan 7, 2020

Choose a reason for hiding this comment

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

I'm mainly concerned about an unintentional breakage, and my hope would be to release this particular change as a patch prior to releasing this PR as a minor.

Copy link
Contributor Author

@arcanis arcanis Jan 7, 2020

Choose a reason for hiding this comment

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

I think #190 has only been released in the 2.0.0-next.0 branch, so for the purpose of the patch release the path.dirname thing can be removed entirely. Do you want me to open a separate PR against the 1.x branch?

Copy link
Member

@ljharb ljharb Jan 7, 2020

Choose a reason for hiding this comment

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

#192 was the PR into the 1.x branch.


function isdir(err, isdir) {
if (err) return cb(err);
if (!isdir) return processDirs(cb, dirs.slice(1));
var file = path.join(dir, x);
loadAsFile(file, opts.package, onfile);
loadAsFile(dir, opts.package, onfile);
}

function onfile(err, m, pkg) {
if (err) return cb(err);
if (m) return cb(null, m, pkg);
loadAsDirectory(path.join(dir, x), opts.package, ondir);
loadAsDirectory(dir, opts.package, ondir);
}

function ondir(err, n, pkg) {
Expand All @@ -287,6 +295,10 @@ module.exports = function resolve(x, options, callback) {
}
}
function loadNodeModules(x, start, cb) {
processDirs(cb, nodeModulesPaths(start, opts, x));
var thunk = function () { return getPackageCandidates(x, start, opts); };
processDirs(
cb,
packageIterator ? packageIterator(x, start, thunk, opts) : thunk()
);
}
};
19 changes: 15 additions & 4 deletions lib/sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ var maybeUnwrapSymlink = function maybeUnwrapSymlink(x, opts) {
return x;
};

var getPackageCandidates = function getPackageCandidates(x, start, opts) {
var dirs = nodeModulesPaths(start, opts, x);
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo Jan 13, 2020

Choose a reason for hiding this comment

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

Since this function is the same in sync.js, maybe it could be extracted to a separate file?

Copy link
Member

@ljharb ljharb Jan 13, 2020

Choose a reason for hiding this comment

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

I thought about that, but then it's a separate file that has to be supported for the rest of v1.

for (var i = 0; i < dirs.length; i++) {
dirs[i] = path.join(dirs[i], x);
}
return dirs;
};

module.exports = function (x, options) {
if (typeof x !== 'string') {
throw new TypeError('Path must be a string.');
Expand All @@ -47,6 +55,7 @@ module.exports = function (x, options) {
var isFile = opts.isFile || defaultIsFile;
var isDirectory = opts.isDirectory || defaultIsDir;
var readFileSync = opts.readFileSync || fs.readFileSync;
var packageIterator = opts.packageIterator;

var extensions = opts.extensions || ['.js'];
var basedir = opts.basedir || path.dirname(caller());
Expand Down Expand Up @@ -162,13 +171,15 @@ module.exports = function (x, options) {
}

function loadNodeModulesSync(x, start) {
var dirs = nodeModulesPaths(start, opts, x);
var thunk = function () { return getPackageCandidates(x, start, opts); };
var dirs = packageIterator ? packageIterator(x, start, thunk, opts) : thunk();

for (var i = 0; i < dirs.length; i++) {
var dir = dirs[i];
if (isDirectory(dir)) {
var m = loadAsFileSync(path.join(dir, '/', x));
if (isDirectory(path.dirname(dir))) {
var m = loadAsFileSync(dir);
if (m) return m;
var n = loadAsDirectorySync(path.join(dir, '/', x));
var n = loadAsDirectorySync(dir);
if (n) return n;
}
}
Expand Down
18 changes: 18 additions & 0 deletions readme.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ options are:
* getNodeModulesDirs - a thunk (no-argument function) that returns the paths using standard `node_modules` resolution
* opts - the resolution options

* `opts.packageIterator(request, start, opts)` - return the list of candidate paths where the packages sources may be found (probably don't use this)
* request - the import specifier being resolved
* start - lookup path
* getPackageCandidates - a thunk (no-argument function) that returns the paths using standard `node_modules` resolution
* opts - the resolution options

* 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 Expand Up @@ -146,6 +152,18 @@ 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)

For advanced users, `paths` can also be a `opts.paths(request, start, opts)` function
* request - the import specifier being resolved
* start - lookup path
* getNodeModulesDirs - a thunk (no-argument function) that returns the paths using standard `node_modules` resolution
* opts - the resolution options

* `opts.packageIterator(request, start, opts)` - return the list of candidate paths where the packages sources may be found (probably don't use this)
* request - the import specifier being resolved
* start - lookup path
* getPackageCandidates - a thunk (no-argument function) that returns the paths using standard `node_modules` resolution
* opts - the resolution options

* 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
16 changes: 16 additions & 0 deletions test/resolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,22 @@ test('other path', function (t) {
});
});

test('path iterator', function (t) {
t.plan(2);

var resolverDir = path.join(__dirname, 'resolver');

var exactIterator = function (x, start, getPackageCandidates, opts) {
return [path.join(resolverDir, x)];
};

resolve('baz', { packageIterator: exactIterator }, function (err, res, pkg) {
if (err) t.fail(err);
t.equal(res, path.join(resolverDir, 'baz/quux.js'));
t.equal(pkg && pkg.name, 'baz');
});
});

test('incorrect main', function (t) {
t.plan(1);

Expand Down
1 change: 1 addition & 0 deletions test/resolver/baz/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{
"name": "baz",
"main": "quux.js"
}
15 changes: 15 additions & 0 deletions test/resolver_sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,21 @@ test('other path', function (t) {
t.end();
});

test('path iterator', function (t) {
var resolverDir = path.join(__dirname, 'resolver');

var exactIterator = function (x, start, getPackageCandidates, opts) {
return [path.join(resolverDir, x)];
};

t.equal(
resolve.sync('baz', { packageIterator: exactIterator }),
path.join(resolverDir, 'baz/quux.js')
);

t.end();
});

test('incorrect main', function (t) {
var resolverDir = path.join(__dirname, 'resolver');
var dir = path.join(resolverDir, 'incorrect_main');
Expand Down