8000 module: throw an error for invalid package.json main entries · nodejs/node@115f0f5 · GitHub
[go: up one dir, main page]
Skip to content

Commit 115f0f5

Browse files
BridgeARBridgeAR
committed
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: #26823 Fixes: #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>
1 parent 7bddfcc commit 115f0f5

File tree

9 files changed

+101
-28
lines changed

9 files changed

+101
-28
lines changed

‎doc/api/deprecations.md‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2400,6 +2400,22 @@ Please use the publicly documented [`timeout.refresh()`][] instead.
24002400
If unreferencing the timeout is necessary, [`timeout.unref()`][] can be used
24012401
with no performance impact since Node.js 10.
24022402
2403+
<a id="DEP0128"></a>
2404+
### DEP0128: modules with an invalid `main` entry and an `index.js` file
2405+
<!-- YAML
2406+
changes:
2407+
- version: REPLACEME
2408+
pr-url: https://github.com/nodejs/node/pull/26823
2409+
description: Documentation-only.
2410+
-->
2411+
2412+
Type: Documentation-only (supports [`--pending-deprecation`][])
2413+
2414+
Modules that have an invalid `main` entry (e.g., `./does-not-exist.js`) and
2415+
also have an `index.js` file in the top level directory will resolve the
2416+
`index.js` file. That is deprecated and is going to throw an error in future
2417+
Node.js versions.
2418+
24032419
[`--pending-deprecation`]: cli.html#cli_pending_deprecation
24042420
[`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
24052421
[`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array

‎doc/api/modules.md‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,9 +169,12 @@ LOAD_INDEX(X)
169169
LOAD_AS_DIRECTORY(X)
170170
1. If X/package.json is a file,
171171
a. Parse X/package.json, and look for "main" field.
172-
b. let M = X + (json main field)
173-
c. LOAD_AS_FILE(M)
174-
d. LOAD_INDEX(M)
172+
b. If "main" is a falsy value, GOTO 2.
173+
c. let M = X + (json main field)
174+
d. LOAD_AS_FILE(M)
175+
e. LOAD_INDEX(M)
176+
f. LOAD_INDEX(X) DEPRECATED
177+
g. THROW "not found"
175178
2. LOAD_INDEX(X)
176179
177180
LOAD_NODE_MODULES(X, START)

‎lib/internal/modules/cjs/loader.js‎

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ const {
5454
ERR_REQUIRE_ESM
5555
} = require('internal/errors').codes;
5656
const { validateString } = require('internal/validators');
57+
const pendingDeprecation = getOptionValue('--pending-deprecation');
5758

5859
module.exports = Module;
5960

@@ -224,15 +225,41 @@ function readPackage(requestPath) {
224225
}
225226
}
226227

227-
function tryPackage(requestPath, exts, isMain) {
228-
var pkg = readPackage(requestPath);
228+
function tryPackage(requestPath, exts, isMain, originalPath) {
229+
const pkg = readPackage(requestPath);
229230

230-
if (!pkg) return false;
231+
if (!pkg) {
232+
return tryExtensions(path.resolve(requestPath, 'index'), exts, isMain);
233+
}
231234

232-
var filename = path.resolve(requestPath, pkg);
233-
return tryFile(filename, isMain) ||
234-
tryExtensions(filename, exts, isMain) ||
235-
tryExtensions(path.resolve(filename, 'index'), exts, isMain);
235+
const filename = path.resolve(requestPath, pkg);
236+
let actual = tryFile(filename, isMain) ||
237+
tryExtensions(filename, exts, isMain) ||
238+
tryExtensions(path.resolve(filename, 'index'), exts, isMain);
239+
if (actual === false) {
240+
actual = tryExtensions(path.resolve(requestPath, 'index'), exts, isMain);
241+
if (!actual) {
242+
// eslint-disable-next-line no-restricted-syntax
243+
const err = new Error(
244+
`Cannot find module '${filename}'. ` +
245+
'Please verify that the package.json has a valid "main" entry'
246+
);
247+
err.code = 'MODULE_NOT_FOUND';
248+
err.path = path.resolve(requestPath, 'package.json');
249+
err.requestPath = originalPath;
250+
// TODO(BridgeAR): Add the requireStack as well.
251+
throw err;
252+
} else if (pendingDeprecation) {
253+
const jsonPath = path.resolve(requestPath, 'package.json');
254+
process.emitWarning(
255+
`Invalid 'main' field in '${jsonPath}' of '${pkg}'. ` +
256+
'Please either fix that or report it to the module author',
257+
'DeprecationWarning',
258+
'DEP0128'
259+
);
260+
}
261+
}
262+
return actual;
236263
}
237264

238265
// In order to minimize unnecessary lstat() calls,
@@ -351,10 +378,7 @@ Module._findPath = function(request, paths, isMain) {
351378
// try it with each of the extensions at "index"
352379
if (exts === undefined)
353380
exts = Object.keys(Module._extensions);
354-
filename = tryPackage(basePath, exts, isMain);
355-
if (!filename) {
356-
filename = tryExtensions(path.resolve(basePath, 'index'), exts, isMain);
357-
}
381+
filename = tryPackage(basePath, exts, isMain, request);
358382
}
359383

360384
if (filename) {

‎test/common/index.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,11 @@ function _expectWarning(name, expected, code) {
494494
return mustCall((warning) => {
495495
const [ message, code ] = expected.shift();
496496
assert.strictEqual(warning.name, name);
497-
assert.strictEqual(warning.message, message);
497+
if (typeof message === 'string') {
498+
assert.strictEqual(warning.message, message);
499+
} else {
500+
assert(message.test(warning.message));
501+
}
498502
assert.strictEqual(warning.code, code);
499503
}, expected.length);
500504
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"name": "missingmain",
3+
"main": "doesnotexist.js"
4+
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
// This file should not be loaded.
2+
throw new Error('Failed');

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Flags: --pending-deprecation
2+
3+
'use strict';
4+
5+
const common = require('../common');
6+
const assert = require('assert');
7+
8+
common.expectWarning('DeprecationWarning', {
9+
DEP0128: /^Invalid 'main' field in '.+main[/\\]package\.json' of 'doesnotexist\.js'\..+module author/
10+
});
11+
12+
assert.strictEqual(require('../fixtures/packages/missing-main').ok, 'ok');

‎test/parallel/test-require-exceptions.js‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,31 +26,28 @@ const fs = require('fs');
2626
const fixtures = require('../common/fixtures');
2727

2828
// A module with an error in it should throw
29-
assert.throws(function() {
29+
assert.throws(() => {
3030
require(fixtures.path('/throws_error'));
3131
}, /^Error: blah$/);
3232

3333
// Requiring the same module again should throw as well
34-
assert.throws(function() {
34+
assert.throws(() => {
3535
require(fixtures.path('/throws_error'));
3636
}, /^Error: blah$/);
3737

3838
// Requiring a module that does not exist should throw an
3939
// error with its `code` set to MODULE_NOT_FOUND
40-
assertModuleNotFound('/DOES_NOT_EXIST');
40+
assert.throws(
41+
() => require('/DOES_NOT_EXIST'),
42+
{ code: 'MODULE_NOT_FOUND' }
43+
);
4144

4245
assertExists('/module-require/not-found/trailingSlash.js');
4346
assertExists('/module-require/not-found/node_modules/module1/package.json');
44-
assertModuleNotFound('/module-require/not-found/trailingSlash');
45-
46-
function assertModuleNotFound(path) {
47-
assert.throws(function() {
48-
require(fixtures.path(path));
49-
}, function(e) {
50-
assert.strictEqual(e.code, 'MODULE_NOT_FOUND');
51-
return true;
52-
});
53-
}
47+
assert.throws(
48+
() => require('/module-require/not-found/trailingSlash'),
49+
{ code: 'MODULE_NOT_FOUND' }
50+
);
5451

5552
function assertExists(fixture) {
5653
assert(fs.existsSync(fixtures.path(fixture)));

‎test/sequential/test-module-loading.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ const path = require('path');
2929

3030
const backslash = /\\/g;
3131

32+
process.on('warning', common.mustNotCall());
33+
3234
console.error('load test-module-loading.js');
3335

3436
assert.strictEqual(require.main.id, '.');
@@ -105,6 +107,15 @@ assert.strictEqual(require('../fixtures/packages/index').ok, 'ok');
105107
assert.strictEqual(require('../fixtures/packages/main').ok, 'ok');
106108
assert.strictEqual(require('../fixtures/packages/main-index').ok, 'ok');
107109
assert.strictEqual(require('../fixtures/packages/missing-main').ok, 'ok');
110+
assert.throws(
111+
() => require('../fixtures/packages/missing-main-no-index'),
112+
{
113+
code: 'MODULE_NOT_FOUND',
114+
message: /packages[/\\]missing-main-no-index[/\\]doesnotexist\.js'\. Please.+package\.json.+valid "main"/,
115+
path: /fixtures[/\\]packages[/\\]missing-main-no-index[/\\]package\.json/,
116+
requestPath: /^\.\.[/\\]fixtures[/\\]packages[/\\]missing-main-no-index$/
117+
}
118+
);
108119

109120
assert.throws(
110121
function() { require('../fixtures/packages/unparseable'); },

0 commit comments

Comments
 (0)
0