Throwing error for an 'invalid main entry'

[pingrishabh]

Fixes #223 & Possibly Fixes #143 too.

The thrown error now suggests the possibility of an invalid main entry in the package.json.

[ljharb]

Thanks! This will need some regression tests - and I'd want to be convinced (ideally, via tests) that the swallowed errors are only coming from path.resolve(x, pkg.main) and not from loadAsDirectorySync, etc.

[ljharb]

You can also see that tests are currently failing - https://github.com/browserify/resolve/pull/234/checks?check_run_id=1471328733 - which implies that the intentional behavior with an incorrect "main" is to silently fall back to index.js, when present.

Specifically, when I put this directory inside a node_modules folder, and hop into the node repl and do require.resolve('incorrect_main'), i get $PWD/node_modules/incorrect_main/index.js, not an error. When I remove $PWD/node_modules/incorrect_main/index.js, however, I get Error: Cannot find module '$PWD/node_modules/incorrect_main/wrong.js'. Please verify that the package.json has a valid "main" entry.

So, I think you specifically have to only error in the case where an explicit main and an implicit main are not found.

[pingrishabh]

Yep, makes sense to me. I'll focus on isolating explicit and implicit main conditions first. Will fallback here if something doesn't add up.

[pingrishabh]

@ljharb There. That should do the trick. Thoughts?

P.S Not sure why that 1 test is failing. Could you please explain?

[ljharb]

We'll need an async fix also.

[ljharb]

Suggested change

	var m = loadAsFileSync(path.resolve(x, pkg.main));
	if (m) return m;
	var n = loadAsDirectorySync(path.resolve(x, pkg.main));
	if (n) return n;
	else {
	var checkIndex = loadAsFileSync(path.resolve(x, 'index.js'));
	if (checkIndex) return checkIndex;
	else {
	var incorrectMainError = new Error("Cannot find module '" + path.resolve(x, pkg.main) + "'. Please verify that the package.json has a valid \"main\" entry");
	incorrectMainError.code = 'INCORRECT_PACKAGE_MAIN';
	throw incorrectMainError;
	}
	}
	try {
	var mainPath = path.resolve(x, pkg.main);
	var m = loadAsFileSync(mainPath);
	if (m) return m;
	var n = loadAsDirectorySync(mainPath);
	if (n) return n;
	var checkIndex = loadAsFileSync(path.resolve(x, 'index.js'));
	if (checkIndex) return checkIndex;
	} catch (e) {}
	var incorrectMainError = new Error("Cannot find module '" + mainPath + "'. Please verify that the package.json has a valid \"main\" entry");
	incorrectMainError.code = 'INCORRECT_PACKAGE_MAIN';
	throw incorrectMainError;

[pingrishabh]

@ljharb I implemented the changes for async.js as well. Can you do a quick review?

[ljharb]

@Cheesetouched seems good, but tests would really be persuasive :-)

[pingrishabh]

Absolutely. I am on it.

[pingrishabh]

@ljharb See if this looks good to you? Feel free to suggest more changes.

[pingrishabh]

@ljharb Added relevant test cases. Ready for review.

[ljharb]

Seems good! I'm still hesitant to backport this to v1, and will think about it more, but at least we can land this in the next v2 prerelease :-)

[ljharb]

whoops, merged a little early, but i think we're ok :-)

[pingrishabh]

@ljharb Any issues? Happy to take to care of it. Do let me know.

[ljharb]

nah, i just meant to wait til all the status checks had finished, and i pulled the trigger a bit prematurely. everything still passed tho.