Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modules: throw an error for invalid package.json main entries #26823

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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="DEP0XXX"></a>
### DEP0XXX: 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.
mcollina marked this conversation as resolved.
Show resolved Hide resolved

[`--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 @@ -225,15 +226,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) {
guybedford marked this conversation as resolved.
Show resolved Hide resolved
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',
'DEP0XXX'
);
}
}
return actual;
}

// In order to minimize unnecessary lstat() calls,
Expand Down Expand Up @@ -352,10 +379,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"
}
3 changes: 3 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,3 @@
// 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', {
DEP0XXX: /^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