module: throw an error for invalid package.json main entries

We currently ignore invalid `main` entries in package.json files.
This does not seem to be very user friendly as it's certainly an
error if the `main` entry is not a valid file name. So instead of
trying to resolve the file otherwise, throw an error immediately to
improve the user experience.
To keep it backwards compatible `index.js` files in the same directory
as the `package.json` will continue to be resolved instead but that
behavior is now deprecated.

PR-URL: https://github.com/nodejs/node/pull/26823
Fixes: https://github.com/nodejs/node/issues/26588
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

doc/api/deprecations.md

@@ -2400,6 +2400,22 @@ Please use the publicly documented [`timeout.refresh()`][] instead.
 If unreferencing the timeout is necessary, [`timeout.unref()`][] can be used
 with no performance impact since Node.js 10.
 
+<a id="DEP0128"></a>
+### DEP0128: modules with an invalid `main` entry and an `index.js` file
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/26823
+    description: Documentation-only.
+-->
+
+Type: Documentation-only (supports [`--pending-deprecation`][])
+
+Modules that have an invalid `main` entry (e.g., `./does-not-exist.js`) and
+also have an `index.js` file in the top level directory will resolve the
+`index.js` file. That is deprecated and is going to throw an error in future
+Node.js versions.
+
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
 [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array

doc/api/modules.md

@@ -169,9 +169,12 @@ LOAD_INDEX(X)
 LOAD_AS_DIRECTORY(X)
 1. If X/package.json is a file,
    a. Parse X/package.json, and look for "main" field.
-   b. let M = X + (json main field)
-   c. LOAD_AS_FILE(M)
-   d. LOAD_INDEX(M)
+   b. If "main" is a falsy value, GOTO 2.
+   c. let M = X + (json main field)
+   d. LOAD_AS_FILE(M)
+   e. LOAD_INDEX(M)
+   f. LOAD_INDEX(X) DEPRECATED
+   g. THROW "not found"
 2. LOAD_INDEX(X)
 
 LOAD_NODE_MODULES(X, START)

lib/internal/modules/cjs/loader.js

@@ -54,6 +54,7 @@ const {
   ERR_REQUIRE_ESM
 } = require('internal/errors').codes;
 const { validateString } = require('internal/validators');
+const pendingDeprecation = getOptionValue('--pending-deprecation');
 
 module.exports = Module;
 
@@ -224,15 +225,41 @@ function readPackage(requestPath) {
   }
 }
 
-function tryPackage(requestPath, exts, isMain) {
-  var pkg = readPackage(requestPath);
+function tryPackage(requestPath, exts, isMain, originalPath) {
+  const pkg = readPackage(requestPath);
 
-  if (!pkg) return false;
+  if (!pkg) {
+    return tryExtensions(path.resolve(requestPath, 'index'), exts, isMain);
+  }
 
-  var filename = path.resolve(requestPath, pkg);
-  return tryFile(filename, isMain) ||
-         tryExtensions(filename, exts, isMain) ||
-         tryExtensions(path.resolve(filename, 'index'), exts, isMain);
+  const filename = path.resolve(requestPath, pkg);
+  let actual = tryFile(filename, isMain) ||
+    tryExtensions(filename, exts, isMain) ||
+    tryExtensions(path.resolve(filename, 'index'), exts, isMain);
+  if (actual === false) {
+    actual = tryExtensions(path.resolve(requestPath, 'index'), exts, isMain);
+    if (!actual) {
+      // eslint-disable-next-line no-restricted-syntax
+      const err = new Error(
+        `Cannot find module '${filename}'. ` +
+        'Please verify that the package.json has a valid "main" entry'
+      );
+      err.code = 'MODULE_NOT_FOUND';
+      err.path = path.resolve(requestPath, 'package.json');
+      err.requestPath = originalPath;
+      // TODO(BridgeAR): Add the requireStack as well.
+      throw err;
+    } else if (pendingDeprecation) {
+      const jsonPath = path.resolve(requestPath, 'package.json');
+      process.emitWarning(
+        `Invalid 'main' field in '${jsonPath}' of '${pkg}'. ` +
+          'Please either fix that or report it to the module author',
+        'DeprecationWarning',
+        'DEP0128'
+      );
+    }
+  }
+  return actual;
 }
 
 // In order to minimize unnecessary lstat() calls,
@@ -351,10 +378,7 @@ Module._findPath = function(request, paths, isMain) {
       // try it with each of the extensions at "index"
       if (exts === undefined)
         exts = Object.keys(Module._extensions);
-      filename = tryPackage(basePath, exts, isMain);
-      if (!filename) {
-        filename = tryExtensions(path.resolve(basePath, 'index'), exts, isMain);
-      }
+      filename = tryPackage(basePath, exts, isMain, request);
     }
 
     if (filename) {

test/common/index.js

@@ -494,7 +494,11 @@ function _expectWarning(name, expected, code) {
   return mustCall((warning) => {
     const [ message, code ] = expected.shift();
     assert.strictEqual(warning.name, name);
-    assert.strictEqual(warning.message, message);
+    if (typeof message === 'string') {
+      assert.strictEqual(warning.message, message);
+    } else {
+      assert(message.test(warning.message));
+    }
     assert.strictEqual(warning.code, code);
   }, expected.length);
 }

test/fixtures/packages/missing-main-no-index/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "missingmain",
+  "main": "doesnotexist.js"
+}

test/fixtures/packages/missing-main-no-index/stray.js

@@ -0,0 +1,2 @@
+// This file should not be loaded.
+throw new Error('Failed');

test/parallel/test-module-loading-deprecated.js

@@ -0,0 +1,12 @@
+// Flags: --pending-deprecation
+
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+common.expectWarning('DeprecationWarning', {
+  DEP0128: /^Invalid 'main' field in '.+main[/\\]package\.json' of 'doesnotexist\.js'\..+module author/
+});
+
+assert.strictEqual(require('../fixtures/packages/missing-main').ok, 'ok');

test/parallel/test-require-exceptions.js

@@ -26,31 +26,28 @@ const fs = require('fs');
 const fixtures = require('../common/fixtures');
 
 // A module with an error in it should throw
-assert.throws(function() {
+assert.throws(() => {
   require(fixtures.path('/throws_error'));
 }, /^Error: blah$/);
 
 // Requiring the same module again should throw as well
-assert.throws(function() {
+assert.throws(() => {
   require(fixtures.path('/throws_error'));
 }, /^Error: blah$/);
 
 // Requiring a module that does not exist should throw an
 // error with its `code` set to MODULE_NOT_FOUND
-assertModuleNotFound('/DOES_NOT_EXIST');
+assert.throws(
+  () => require('/DOES_NOT_EXIST'),
+  { code: 'MODULE_NOT_FOUND' }
+);
 
 assertExists('/module-require/not-found/trailingSlash.js');
 assertExists('/module-require/not-found/node_modules/module1/package.json');
-assertModuleNotFound('/module-require/not-found/trailingSlash');
-
-function assertModuleNotFound(path) {
-  assert.throws(function() {
-    require(fixtures.path(path));
-  }, function(e) {
-    assert.strictEqual(e.code, 'MODULE_NOT_FOUND');
-    return true;
-  });
-}
+assert.throws(
+  () => require('/module-require/not-found/trailingSlash'),
+  { code: 'MODULE_NOT_FOUND' }
+);
 
 function assertExists(fixture) {
   assert(fs.existsSync(fixtures.path(fixture)));

test/sequential/test-module-loading.js

@@ -29,6 +29,8 @@ const path = require('path');
 
 const backslash = /\\/g;
 
+process.on('warning', common.mustNotCall());
+
 console.error('load test-module-loading.js');
 
 assert.strictEqual(require.main.id, '.');
@@ -105,6 +107,15 @@ assert.strictEqual(require('../fixtures/packages/index').ok, 'ok');
 assert.strictEqual(require('../fixtures/packages/main').ok, 'ok');
 assert.strictEqual(require('../fixtures/packages/main-index').ok, 'ok');
 assert.strictEqual(require('../fixtures/packages/missing-main').ok, 'ok');
+assert.throws(
+  () => require('../fixtures/packages/missing-main-no-index'),
+  {
+    code: 'MODULE_NOT_FOUND',
+    message: /packages[/\\]missing-main-no-index[/\\]doesnotexist\.js'\. Please.+package\.json.+valid "main"/,
+    path: /fixtures[/\\]packages[/\\]missing-main-no-index[/\\]package\.json/,
+    requestPath: /^\.\.[/\\]fixtures[/\\]packages[/\\]missing-main-no-index$/
+  }
+);
 
 assert.throws(
   function() { require('../fixtures/packages/unparseable'); },