Skip to content

Commit

Permalink
module: throw an error for invalid package.json main entries
Browse files Browse the repository at this point in the history
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: #26823
Fixes: #26588
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
  • Loading branch information
BridgeAR committed Mar 27, 2019
1 parent 7bddfcc commit 115f0f5
Show file tree
Hide file tree
Showing 9 changed files with 101 additions and 28 deletions.
16 changes: 16 additions & 0 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 6 additions & 3 deletions doc/api/modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
46 changes: 35 additions & 11 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 5 additions & 1 deletion test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/packages/missing-main-no-index/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "missingmain",
"main": "doesnotexist.js"
}
2 changes: 2 additions & 0 deletions test/fixtures/packages/missing-main-no-index/stray.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// This file should not be loaded.
throw new Error('Failed');
12 changes: 12 additions & 0 deletions test/parallel/test-module-loading-deprecated.js
Original file line number Diff line number Diff line change
@@ -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');
23 changes: 10 additions & 13 deletions test/parallel/test-require-exceptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Expand Down
11 changes: 11 additions & 0 deletions test/sequential/test-module-loading.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, '.');
Expand Down Expand Up @@ -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'); },
Expand Down

0 comments on commit 115f0f5

Please sign in to comment.