Omitting the package option causes resolve to eagerly look for package.json files

[keithamus]

Given a file structure like the following:

.
├── a
│   └── bar.js
└── b
    └── baz.js

If bar were to require('../b/baz') with then we should expect the following stat calls to be made:

b/baz
b/baz.mjs // only on 2.x branch
b/baz.js

However, this only happens if the package: {} option is present (package being set to any truthy value).

If we try to run without the package: {} option the following files are stated:

b/package.json
b/baz
b/package.json
b/baz.mjs // only on 2.x branch
b/baz.js

Here's a test you can add to test/mock.js to prove this out:

test('mock and assert each stat call', function (t) {
    t.plan(3);

    var files = {};
    files[path.resolve('/a/bar.js')] = 'beep';
    files[path.resolve('/b/baz.js')] = 'beepbeep';

    var statSteps = [];
    var readSteps = [];

    function opts(basedir) {
        return {
            basedir: '/a',
            extensions: ['.js'],
            isFile: function (file, cb) {
                statSteps.push(file);
                cb(null, Object.prototype.hasOwnProperty.call(files, path.resolve(file)));
            },
            readFile: function (file, cb) {
                readSteps.push(file);
                cb(null, files[path.resolve(file)]);
            }
        };
    }

    resolve('../b/baz', opts('/a'), function (err, res, pkg) {
        if (err) return t.fail(err);
        t.equal(res, path.resolve('/b/baz.js'));
        t.deepEqual(statSteps, [
            '/b/baz',
            '/b/baz.js'
        ]);
        t.deepEqual(readSteps, []);
    });
});

A "fix" would be to simply default the package argument to an empty object:

diff --git lib/async.js lib/async.js
--- lib/async.js
+++ lib/async.js
@@ -58,8 +58,8 @@ module.exports = function resolve(x, options, callback) {
             res = path.resolve(basedir, x);
             if (x === '..' || x.slice(-1) === '/') res += '/';
             if ((/\/$/).test(x) && res === basedir) {
-                loadAsDirectory(res, opts.package, onfile);
-            } else loadAsFile(res, opts.package, onfile);
+                loadAsDirectory(res, opts.package || {}, onfile);
+            } else loadAsFile(res, opts.package || {}, onfile);
         } else loadNodeModules(x, basedir, function (err, n, pkg) {
             if (err) cb(err);
             else if (n) cb(null, n, pkg);

However this breaks quite a few tests.

This option has existed for a long time and the behaviour looks to be the same dating back many years. From my guessing this option seems to exist because in some use cases for browserify the package.json browser field can influence what gets resolved?

[ljharb]

This definitely predates my maintainership of this package.

Absolutely yes, though - every file within a package could be replaced by a key in a browser field in package.json, so browserify definitely would need to always check every package.json, even for deep imports.

Note as well, that package.json will always need to be checked for every .js import with the current flagged modules implementation - so i don't think minimizing package.json stat calls is really in the cards going forward, altho it'd be ideal for resolve to match CJS behavior as closely as possible.

[ljharb]

This seems answered; happy to reopen if there's something actionable for this package.