Skip to content

Commit 9863d27

Browse files
joyeecheungtargos
joyeecheung
authored and
targos
committedDec 13, 2024
module: only emit require(esm) warning under --trace-require-module
require(esm) is relatively stable now and the experimental warning has run its course - it's now more troublesome than useful. This patch changes it to no longer emit a warning unless `--trace-require-module` is explicitly used. The flag supports two modes: - `--trace-require-module=all`: emit warnings for all usages - `--trace-require-module=no-node-modules`: emit warnings for usages that do not come from a `node_modules` folder. PR-URL: #56194 Fixes: #55417 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
·
v23.11.0v23.5.0
1 parent 8325fa5 commit 9863d27

11 files changed

+80
-48
lines changed
 

‎doc/api/cli.md

+13
Original file line numberDiff line numberDiff line change
@@ -2613,6 +2613,18 @@ added:
26132613
Prints a stack trace whenever an environment is exited proactively,
26142614
i.e. invoking `process.exit()`.
26152615

2616+
### `--trace-require-module=mode`
2617+
2618+
<!-- YAML
2619+
added:
2620+
- REPLACEME
2621+
-->
2622+
2623+
Prints information about usage of [Loading ECMAScript modules using `require()`][].
2624+
2625+
When `mode` is `all`, all usage is printed. When `mode` is `no-node-modules`, usage
2626+
from the `node_modules` folder is excluded.
2627+
26162628
### `--trace-sigint`
26172629

26182630
<!-- YAML
@@ -3123,6 +3135,7 @@ one is included in the list below.
31233135
* `--trace-event-file-pattern`
31243136
* `--trace-events-enabled`
31253137
* `--trace-exit`
3138+
* `--trace-require-module`
31263139
* `--trace-sigint`
31273140
* `--trace-sync-io`
31283141
* `--trace-tls`

‎doc/api/modules.md

+8-3
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,11 @@ added:
175175
- v22.0.0
176176
- v20.17.0
177177
changes:
178+
- version:
179+
- REPLACEME
180+
pr-url: https://github.com/nodejs/node/pull/56194
181+
description: This feature no longer emits an experimental warning by default,
182+
though the warning can still be emitted by --trace-require-module.
178183
- version: v23.0.0
179184
pr-url: https://github.com/nodejs/node/pull/55085
180185
description: This feature is no longer behind the `--experimental-require-module` CLI flag.
@@ -315,9 +320,8 @@ help users fix them.
315320

316321
Support for loading ES modules using `require()` is currently
317322
experimental and can be disabled using `--no-experimental-require-module`.
318-
When `require()` actually encounters an ES module for the
319-
first time in the process, it will emit an experimental warning. The
320-
warning is expected to be removed when this feature stablizes.
323+
To print where this feature is used, use [`--trace-require-module`][].
324+
321325
This feature can be detected by checking if
322326
[`process.features.require_module`][] is `true`.
323327

@@ -1267,6 +1271,7 @@ This section was moved to
12671271
[GLOBAL_FOLDERS]: #loading-from-the-global-folders
Has a conversation. Original line has a conversation.
12681272
[`"main"`]: packages.md#main
12691273
[`"type"`]: packages.md#type
1274+
[`--trace-require-module`]: cli.md#--trace-require-modulemode
12701275
[`ERR_REQUIRE_ASYNC_MODULE`]: errors.md#err_require_async_module
12711276
[`ERR_UNSUPPORTED_DIR_IMPORT`]: errors.md#err_unsupported_dir_import
12721277
[`MODULE_NOT_FOUND`]: errors.md#module_not_found

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

+17-12
Original file line numberDiff line numberDiff line change
@@ -1499,7 +1499,7 @@ Module.prototype.require = function(id) {
14991499
}
15001500
};
15011501

1502-
let emittedRequireModuleWarning = false;
1502+
let requireModuleWarningMode;
15031503
/**
15041504
* Resolve and evaluate it synchronously as ESM if it's ESM.
15051505
* @param {Module} mod CJS module instance
@@ -1520,17 +1520,22 @@ function loadESMFromCJS(mod, filename, format, source) {
15201520
} else {
15211521
const parent = mod[kModuleParent];
15221522

1523-
if (!emittedRequireModuleWarning) {
1523+
requireModuleWarningMode ??= getOptionValue('--trace-require-module');
1524+
if (requireModuleWarningMode) {
15241525
let shouldEmitWarning = false;
1525-
// Check if the require() comes from node_modules.
1526-
if (parent) {
1527-
shouldEmitWarning = !isUnderNodeModules(parent.filename);
1528-
} else if (mod[kIsCachedByESMLoader]) {
1529-
// It comes from the require() built for `import cjs` and doesn't have a parent recorded
1530-
// in the CJS module instance. Inspect the stack trace to see if the require()
1531-
// comes from node_modules and reduce the noise. If there are more than 100 frames,
1532-
// just give up and assume it is under node_modules.
1533-
shouldEmitWarning = !isInsideNodeModules(100, true);
1526+
if (requireModuleWarningMode === 'no-node-modules') {
1527+
// Check if the require() comes from node_modules.
1528+
if (parent) {
1529+
shouldEmitWarning = !isUnderNodeModules(parent.filename);
1530+
} else if (mod[kIsCachedByESMLoader]) {
1531+
// It comes from the require() built for `import cjs` and doesn't have a parent recorded
1532+
// in the CJS module instance. Inspect the stack trace to see if the require()
1533+
// comes from node_modules and reduce the noise. If there are more than 100 frames,
1534+
// just give up and assume it is under node_modules.
1535+
shouldEmitWarning = !isInsideNodeModules(100, true);
1536+
}
1537+
} else {
1538+
shouldEmitWarning = true;
15341539
}
15351540
if (shouldEmitWarning) {
15361541
let messagePrefix;
@@ -1556,7 +1561,7 @@ function loadESMFromCJS(mod, filename, format, source) {
15561561
messagePrefix,
15571562
undefined,
15581563
parent?.require);
1559-
emittedRequireModuleWarning = true;
1564+
requireModuleWarningMode = true;
15601565
}
15611566
}
15621567
const {

‎src/node_options.cc

+12
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,11 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors,
135135
errors->push_back("--heapsnapshot-near-heap-limit must not be negative");
136136
}
137137

138+
if (!trace_require_module.empty() && trace_require_module != "all" &&
139+
trace_require_module != "no-node-modules") {
140+
errors->push_back("invalid value for --trace-require-module");
141+
}
142+
138143
if (test_runner) {
139144
if (test_isolation == "none") {
140145
debug_options_.allow_attaching_debugger = true;
@@ -770,6 +775,13 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
770775
&EnvironmentOptions::trace_env_native_stack,
771776
kAllowedInEnvvar);
772777

778+
AddOption(
779+
"--trace-require-module",
780+
"Print access to require(esm). Options are 'all' (print all usage) and "
781+
"'no-node-modules' (excluding usage from the node_modules folder)",
782+
&EnvironmentOptions::trace_require_module,
783+
kAllowedInEnvvar);
784+
773785
AddOption("--extra-info-on-fatal-exception",
774786
"hide extra information on fatal exception that causes exit",
775787
&EnvironmentOptions::extra_info_on_fatal_exception,

‎src/node_options.h

+1
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,7 @@ class EnvironmentOptions : public Options {
210210
bool trace_env = false;
211211
bool trace_env_js_stack = false;
212212
bool trace_env_native_stack = false;
213+
std::string trace_require_module;
213214
bool extra_info_on_fatal_exception = true;
214215
std::string unhandled_rejections;
215216
std::vector<std::string> userland_loaders;

‎test/es-module/test-require-module-preload.js

-11
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@ const { spawnSyncAndAssert } = require('../common/child_process');
55
const { fixturesDir } = require('../common/fixtures');
66

77
function testPreload(preloadFlag) {
8-
// The warning is only emitted when ESM is loaded by --require.
9-
const isRequire = preloadFlag === '--require';
108
// Test named exports.
119
{
1210
spawnSyncAndAssert(
@@ -22,8 +20,6 @@ function testPreload(preloadFlag) {
2220
},
2321
{
2422
stdout: 'A',
25-
stderr: isRequire ?
26-
/ExperimentalWarning: --require is loading ES Module .*module-named-exports\.mjs using require/ : undefined,
2723
trim: true,
2824
}
2925
);
@@ -43,8 +39,6 @@ function testPreload(preloadFlag) {
4339
cwd: fixturesDir
4440
},
4541
{
46-
stderr: isRequire ?
47-
/ExperimentalWarning: --require is loading ES Module .*import-esm\.mjs using require/ : undefined,
4842
stdout: /^world\s+A$/,
4943
trim: true,
5044
}
@@ -66,8 +60,6 @@ function testPreload(preloadFlag) {
6660
},
6761
{
6862
stdout: /^ok\s+A$/,
69-
stderr: isRequire ?
70-
/ExperimentalWarning: --require is loading ES Module .*cjs-exports\.mjs using require/ : undefined,
7163
trim: true,
7264
}
7365
);
@@ -90,8 +82,6 @@ function testPreload(preloadFlag) {
9082
},
9183
{
9284
stdout: /^world\s+A$/,
93-
stderr: isRequire ?
94-
/ExperimentalWarning: --require is loading ES Module .*require-cjs\.mjs using require/ : undefined,
9585
trim: true,
9686
}
9787
);
@@ -117,7 +107,6 @@ testPreload('--import');
117107
},
118108
{
119109
stdout: /^package-type-module\s+A$/,
120-
stderr: /ExperimentalWarning: --require is loading ES Module .*package-type-module[\\/]index\.js using require/,
121110
trim: true,
122111
}
123112
);

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

+11-3
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
'use strict';
22

3-
// This checks the warning and the stack trace emitted by the require(esm)
4-
// experimental warning. It can get removed when `require(esm)` becomes stable.
5-
3+
// This checks the warning and the stack trace emitted by --trace-require-module=all.
64
require('../common');
75
const { spawnSyncAndAssert } = require('../common/child_process');
86
const fixtures = require('../common/fixtures');
97
const assert = require('assert');
108

119
spawnSyncAndAssert(process.execPath, [
1210
'--trace-warnings',
11+
'--trace-require-module=all',
1312
fixtures.path('es-modules', 'require-module.js'),
1413
], {
1514
trim: true,
@@ -33,3 +32,12 @@ spawnSyncAndAssert(process.execPath, [
3332
);
3433
}
3534
});
35+
36+
spawnSyncAndAssert(process.execPath, [
37+
'--trace-require-module=1',
38+
fixtures.path('es-modules', 'require-module.js'),
39+
], {
40+
status: 9,
41+
trim: true,
42+
stderr: /invalid value for --trace-require-module/
43+
});

‎test/es-module/test-require-module.js

-10
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,6 @@
33

44
const common = require('../common');
55
const assert = require('assert');
6-
const path = require('path');
7-
8-
// Only the first load will trigger the warning.
9-
common.expectWarning(
10-
'ExperimentalWarning',
11-
`CommonJS module ${__filename} is loading ES Module ` +
12-
`${path.resolve(__dirname, '../fixtures/es-module-loaders/module-named-exports.mjs')} using require().\n` +
13-
'Support for loading ES Module in require() is an experimental feature ' +
14-
'and might change at any time'
15-
);
166

177
// Test named exports.
188
{

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

+18-6
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
'use strict';
22

3-
// This checks the experimental warning for require(esm) is disabled when the
4-
// require() comes from node_modules.
3+
// This checks the warning and the stack trace emitted by
4+
// --trace-require-module=no-node-modules.
55
require('../common');
66
const { spawnSyncAndAssert } = require('../common/child_process');
77
const fixtures = require('../common/fixtures');
@@ -14,7 +14,10 @@ const warningRE = /Support for loading ES Module in require\(\)/;
1414
// require() in non-node_modules -> esm in node_modules should warn.
1515
spawnSyncAndAssert(
1616
process.execPath,
17-
[fixtures.path('es-modules', 'test_node_modules', 'require-esm.js')],
17+
[
18+
'--trace-require-module=no-node-modules',
19+
fixtures.path('es-modules', 'test_node_modules', 'require-esm.js'),
20+
],
1821
{
1922
trim: true,
2023
stderr: warningRE,
@@ -26,7 +29,10 @@ spawnSyncAndAssert(
2629
// should not warn.
2730
spawnSyncAndAssert(
2831
process.execPath,
29-
[fixtures.path('es-modules', 'test_node_modules', 'require-require-esm.js')],
32+
[
33+
'--trace-require-module=no-node-modules',
34+
fixtures.path('es-modules', 'test_node_modules', 'require-require-esm.js'),
35+
],
3036
{
3137
trim: true,
3238
stderr: '',
@@ -38,7 +44,10 @@ spawnSyncAndAssert(
3844
// should not warn.
3945
spawnSyncAndAssert(
4046
process.execPath,
41-
[fixtures.path('es-modules', 'test_node_modules', 'import-require-esm.mjs')],
47+
[
48+
'--trace-require-module=no-node-modules',
49+
fixtures.path('es-modules', 'test_node_modules', 'import-require-esm.mjs'),
50+
],
4251
{
4352
trim: true,
4453
stderr: '',
@@ -50,7 +59,10 @@ spawnSyncAndAssert(
5059
// require() in node_modules -> esm in node_modules should not warn.
5160
spawnSyncAndAssert(
5261
process.execPath,
53-
[fixtures.path('es-modules', 'test_node_modules', 'import-import-require-esm.mjs')],
62+
[
63+
'--trace-require-module=no-node-modules',
64+
fixtures.path('es-modules', 'test_node_modules', 'import-import-require-esm.mjs'),
65+
],
5466
{
5567
trim: true,
5668
stderr: '',

‎test/es-module/test-typescript-commonjs.mjs

-1
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,6 @@ test('execute a .cts file importing a .mts file export', async () => {
116116
fixtures.path('typescript/cts/test-require-mts-module.cts'),
117117
]);
118118

119-
match(result.stderr, /Support for loading ES Module in require\(\) is an experimental feature and might change at any time/);
120119
match(result.stdout, /Hello, TypeScript!/);
121120
strictEqual(result.code, 0);
122121
});

‎test/es-module/test-typescript.mjs

-2
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,6 @@ test('expect failure of a TypeScript file requiring ES module syntax', async ()
175175
fixtures.path('typescript/ts/test-require-module.ts'),
176176
]);
177177

178-
match(result.stderr, /Support for loading ES Module in require\(\) is an experimental feature and might change at any time/);
179178
match(result.stdout, /Hello, TypeScript!/);
180179
strictEqual(result.code, 0);
181180
});
@@ -231,7 +230,6 @@ test('execute a TypeScript file with CommonJS syntax requiring .mts using requir
231230
fixtures.path('typescript/ts/test-require-mts.ts'),
232231
]);
233232

234-
match(result.stderr, /Support for loading ES Module in require\(\) is an experimental feature and might change at any time/);
235233
match(result.stdout, /Hello, TypeScript!/);
236234
strictEqual(result.code, 0);
237235
});

0 commit comments

Comments
 (0)
Please sign in to comment.