From 115f0f5a57f50f6b039f28a56910207f92df116d Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 20 Mar 2019 17:00:57 +0100 Subject: [PATCH] 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 Reviewed-By: Richard Lau Reviewed-By: Rich Trott Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- doc/api/deprecations.md | 16 +++++++ doc/api/modules.md | 9 ++-- lib/internal/modules/cjs/loader.js | 46 ++++++++++++++----- test/common/index.js | 6 ++- .../missing-main-no-index/package.json | 4 ++ .../packages/missing-main-no-index/stray.js | 2 + .../test-module-loading-deprecated.js | 12 +++++ test/parallel/test-require-exceptions.js | 23 ++++------ test/sequential/test-module-loading.js | 11 +++++ 9 files changed, 101 insertions(+), 28 deletions(-) create mode 100644 test/fixtures/packages/missing-main-no-index/package.json create mode 100644 test/fixtures/packages/missing-main-no-index/stray.js create mode 100644 test/parallel/test-module-loading-deprecated.js diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index b23ee43ba53c87..9820b8039c0792 100644 --- a/doc/api/deprecations.md +++ b/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. + +### DEP0128: modules with an invalid `main` entry and an `index.js` file + + +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 diff --git a/doc/api/modules.md b/doc/api/modules.md index 5ef61141a6353c..99c2ab5fd4eff5 100644 --- a/doc/api/modules.md +++ b/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) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index ea1faf9b7aa033..f205b47a4a2798 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/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) { diff --git a/test/common/index.js b/test/common/index.js index 94b90ac27ba034..d6a24dd944f82a 100644 --- a/test/common/index.js +++ b/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); } diff --git a/test/fixtures/packages/missing-main-no-index/package.json b/test/fixtures/packages/missing-main-no-index/package.json new file mode 100644 index 00000000000000..feb846703ff796 --- /dev/null +++ b/test/fixtures/packages/missing-main-no-index/package.json @@ -0,0 +1,4 @@ +{ + "name": "missingmain", + "main": "doesnotexist.js" +} diff --git a/test/fixtures/packages/missing-main-no-index/stray.js b/test/fixtures/packages/missing-main-no-index/stray.js new file mode 100644 index 00000000000000..3960ed1a9d9159 --- /dev/null +++ b/test/fixtures/packages/missing-main-no-index/stray.js @@ -0,0 +1,2 @@ +// This file should not be loaded. +throw new Error('Failed'); diff --git a/test/parallel/test-module-loading-deprecated.js b/test/parallel/test-module-loading-deprecated.js new file mode 100644 index 00000000000000..3aa81eea6e906a --- /dev/null +++ b/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'); diff --git a/test/parallel/test-require-exceptions.js b/test/parallel/test-require-exceptions.js index 132bffed0f6529..70d6b8334a6a2a 100644 --- a/test/parallel/test-require-exceptions.js +++ b/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))); diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js index 0269445c76c5ce..630830f5dddff6 100644 --- a/test/sequential/test-module-loading.js +++ b/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'); },