Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit e01dd4b

Browse filesBrowse files
joyeecheungmarco-ippolito
joyeecheung
authored and
marco-ippolito
committed
module: do not warn when require(esm) comes from node_modules
As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: #55960 Backport-PR-URL: #56927 Refs: #55217 Refs: #52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>
1 parent 011e6e0 commit e01dd4b
Copy full SHA for e01dd4b

File tree

Expand file treeCollapse file tree

11 files changed

+120
-20
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

11 files changed

+120
-20
lines changed
Open diff view settings
Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/cjs/loader.js
+40-20Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ const {
7070
module_export_private_symbol,
7171
module_parent_private_symbol,
7272
},
73+
isInsideNodeModules,
7374
} = internalBinding('util');
7475

7576
const { kEvaluated, createRequiredModuleFacade } = internalBinding('module_wrap');
@@ -126,6 +127,7 @@ const {
126127
kEmptyObject,
127128
setOwnProperty,
128129
getLazy,
130+
isUnderNodeModules,
129131
} = require('internal/util');
130132
const {
131133
makeContextifyScript,
@@ -1314,6 +1316,7 @@ Module.prototype.require = function(id) {
13141316
}
13151317
};
13161318

1319+
let emittedRequireModuleWarning = false;
13171320
/**
13181321
* Resolved path to `process.argv[1]` will be lazily placed here
13191322
* (needed for setting breakpoint when called with `--inspect-brk`).
@@ -1341,29 +1344,46 @@ function loadESMFromCJS(mod, filename) {
13411344
setOwnProperty(process, 'mainModule', undefined);
13421345
} else {
13431346
const parent = mod[kModuleParent];
1344-
let messagePrefix;
1345-
if (parent) {
1346-
// In the case of the module calling `require()`, it's more useful to know its absolute path.
1347-
let from = parent.filename || parent.id;
1348-
// In the case of the module being require()d, it's more useful to know the id passed into require().
1349-
const to = mod.id || mod.filename;
1350-
if (from === 'internal/preload') {
1351-
from = '--require';
1352-
} else if (from === '<repl>') {
1353-
from = 'The REPL';
1354-
} else if (from === '.') {
1355-
from = 'The entry point';
1356-
} else {
1357-
from &&= `CommonJS module ${from}`;
1347+
1348+
if (!emittedRequireModuleWarning) {
1349+
let shouldEmitWarning = false;
1350+
// Check if the require() comes from node_modules.
1351+
if (parent) {
1352+
shouldEmitWarning = !isUnderNodeModules(parent.filename);
1353+
} else if (mod[kIsCachedByESMLoader]) {
1354+
// It comes from the require() built for `import cjs` and doesn't have a parent recorded
1355+
// in the CJS module instance. Inspect the stack trace to see if the require()
1356+
// comes from node_modules and reduce the noise. If there are more than 100 frames,
1357+
// just give up and assume it is under node_modules.
1358+
shouldEmitWarning = !isInsideNodeModules(100, true);
13581359
}
1359-
if (from && to) {
1360-
messagePrefix = `${from} is loading ES Module ${to} using require().\n`;
1360+
if (shouldEmitWarning) {
1361+
let messagePrefix;
1362+
if (parent) {
1363+
// In the case of the module calling `require()`, it's more useful to know its absolute path.
1364+
let from = parent.filename || parent.id;
1365+
// In the case of the module being require()d, it's more useful to know the id passed into require().
1366+
const to = mod.id || mod.filename;
1367+
if (from === 'internal/preload') {
1368+
from = '--require';
1369+
} else if (from === '<repl>') {
1370+
from = 'The REPL';
1371+
} else if (from === '.') {
1372+
from = 'The entry point';
1373+
} else {
1374+
from &&= `CommonJS module ${from}`;
1375+
}
1376+
if (from && to) {
1377+
messagePrefix = `${from} is loading ES Module ${to} using require().\n`;
1378+
}
1379+
}
1380+
emitExperimentalWarning('Support for loading ES Module in require()',
1381+
messagePrefix,
1382+
undefined,
1383+
parent?.require);
1384+
emittedRequireModuleWarning = true;
13611385
}
13621386
}
1363-
emitExperimentalWarning('Support for loading ES Module in require()',
1364-
messagePrefix,
1365-
undefined,
1366-
parent?.require);
13671387
const {
13681388
wrap,
13691389
namespace,
Collapse file

‎test/es-module/test-require-node-modules-warning.js‎

Copy file name to clipboard
+59Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
'use strict';
2+
3+
// This checks the experimental warning for require(esm) is disabled when the
4+
// require() comes from node_modules.
5+
require('../common');
6+
const { spawnSyncAndAssert } = require('../common/child_process');
7+
const fixtures = require('../common/fixtures');
8+
9+
const warningRE = /Support for loading ES Module in require\(\)/;
10+
11+
// The fixtures are placed in a directory that includes "node_modules" in its name
12+
// to check false negatives.
13+
14+
// require() in non-node_modules -> esm in node_modules should warn.
15+
spawnSyncAndAssert(
16+
process.execPath,
17+
[fixtures.path('es-modules', 'test_node_modules', 'require-esm.js')],
18+
{
19+
trim: true,
20+
stderr: warningRE,
21+
stdout: 'world',
22+
}
23+
);
24+
25+
// require() in non-node_modules -> require() in node_modules -> esm in node_modules
26+
// should not warn.
27+
spawnSyncAndAssert(
28+
process.execPath,
29+
[fixtures.path('es-modules', 'test_node_modules', 'require-require-esm.js')],
30+
{
31+
trim: true,
32+
stderr: '',
33+
stdout: 'world',
34+
}
35+
);
36+
37+
// Import in non-node_modules -> require() in node_modules -> esm in node_modules
38+
// should not warn.
39+
spawnSyncAndAssert(
40+
process.execPath,
41+
[fixtures.path('es-modules', 'test_node_modules', 'import-require-esm.mjs')],
42+
{
43+
trim: true,
44+
stderr: '',
45+
stdout: 'world',
46+
}
47+
);
48+
49+
// Import in non-node_modules -> import in node_modules ->
50+
// require() in node_modules -> esm in node_modules should not warn.
51+
spawnSyncAndAssert(
52+
process.execPath,
53+
[fixtures.path('es-modules', 'test_node_modules', 'import-import-require-esm.mjs')],
54+
{
55+
trim: true,
56+
stderr: '',
57+
stdout: 'world',
58+
}
59+
);
Collapse file

‎test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs‎

Copy file name to clipboardExpand all lines: test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs
+2Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/fixtures/es-modules/test_node_modules/import-require-esm.mjs‎

Copy file name to clipboardExpand all lines: test/fixtures/es-modules/test_node_modules/import-require-esm.mjs
+2Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js‎

Copy file name to clipboardExpand all lines: test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js
+1Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json‎

Copy file name to clipboardExpand all lines: test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json
+4Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js‎

Copy file name to clipboardExpand all lines: test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js
+2Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json‎

Copy file name to clipboardExpand all lines: test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json
+4Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js‎

Copy file name to clipboardExpand all lines: test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js
+2Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Collapse file

‎test/fixtures/es-modules/test_node_modules/require-esm.js‎

Copy file name to clipboardExpand all lines: test/fixtures/es-modules/test_node_modules/require-esm.js
+2Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.