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

module: only emit require(esm) warning under --trace-require-module #56194

Closed
wants to merge 2 commits 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
13 changes: 13 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -2616,6 +2616,18 @@ added:
Prints a stack trace whenever an environment is exited proactively,
i.e. invoking `process.exit()`.

### `--trace-require-module=mode`

<!-- YAML
added:
- REPLACEME
-->

Prints information about usage of [Loading ECMAScript modules using `require()`][].

When `mode` is `all`, all usage is printed. When `mode` is `no-node-modules`, usage
from the `node_modules` folder is excluded.

### `--trace-sigint`

<!-- YAML
Expand Down Expand Up @@ -3131,6 +3143,7 @@ one is included in the list below.
* `--trace-event-file-pattern`
* `--trace-events-enabled`
* `--trace-exit`
* `--trace-require-module`
* `--trace-sigint`
* `--trace-sync-io`
* `--trace-tls`
Expand Down
11 changes: 8 additions & 3 deletions doc/api/modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,11 @@ added:
- v22.0.0
- v20.17.0
changes:
- version:
- REPLACEME
pr-url: https://github.com/nodejs/node/pull/56194
description: This feature no longer emits an experimental warning by default,
though the warning can still be emitted by --trace-require-module.
- version:
- v23.0.0
- v22.12.0
Expand Down Expand Up @@ -319,9 +324,8 @@ help users fix them.

Support for loading ES modules using `require()` is currently
experimental and can be disabled using `--no-experimental-require-module`.
When `require()` actually encounters an ES module for the
first time in the process, it will emit an experimental warning. The
warning is expected to be removed when this feature stablizes.
To print where this feature is used, use [`--trace-require-module`][].

This feature can be detected by checking if
[`process.features.require_module`][] is `true`.

Expand Down Expand Up @@ -1271,6 +1275,7 @@ This section was moved to
[GLOBAL_FOLDERS]: #loading-from-the-global-folders
[`"main"`]: packages.md#main
[`"type"`]: packages.md#type
[`--trace-require-module`]: cli.md#--trace-require-modulemode
[`ERR_REQUIRE_ASYNC_MODULE`]: errors.md#err_require_async_module
[`ERR_UNSUPPORTED_DIR_IMPORT`]: errors.md#err_unsupported_dir_import
[`MODULE_NOT_FOUND`]: errors.md#module_not_found
Expand Down
29 changes: 17 additions & 12 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -1330,7 +1330,7 @@ Module.prototype.require = function(id) {
}
};

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

if (!emittedRequireModuleWarning) {
requireModuleWarningMode ??= getOptionValue('--trace-require-module');
if (requireModuleWarningMode) {
let shouldEmitWarning = false;
// Check if the require() comes from node_modules.
if (parent) {
shouldEmitWarning = !isUnderNodeModules(parent.filename);
} else if (mod[kIsCachedByESMLoader]) {
// It comes from the require() built for `import cjs` and doesn't have a parent recorded
// in the CJS module instance. Inspect the stack trace to see if the require()
// comes from node_modules and reduce the noise. If there are more than 100 frames,
// just give up and assume it is under node_modules.
shouldEmitWarning = !isInsideNodeModules(100, true);
if (requireModuleWarningMode === 'no-node-modules') {
// Check if the require() comes from node_modules.
if (parent) {
shouldEmitWarning = !isUnderNodeModules(parent.filename);
} else if (mod[kIsCachedByESMLoader]) {
// It comes from the require() built for `import cjs` and doesn't have a parent recorded
// in the CJS module instance. Inspect the stack trace to see if the require()
// comes from node_modules and reduce the noise. If there are more than 100 frames,
// just give up and assume it is under node_modules.
shouldEmitWarning = !isInsideNodeModules(100, true);
}
} else {
shouldEmitWarning = true;
}
if (shouldEmitWarning) {
let messagePrefix;
Expand All @@ -1387,7 +1392,7 @@ function loadESMFromCJS(mod, filename, format, source) {
messagePrefix,
undefined,
parent?.require);
emittedRequireModuleWarning = true;
requireModuleWarningMode = true;
}
}
const {
Expand Down
12 changes: 12 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors,
errors->push_back("--heapsnapshot-near-heap-limit must not be negative");
}

if (!trace_require_module.empty() && trace_require_module != "all" &&
trace_require_module != "no-node-modules") {
errors->push_back("invalid value for --trace-require-module");
}

if (test_runner) {
if (test_isolation == "none") {
debug_options_.allow_attaching_debugger = true;
Expand Down Expand Up @@ -771,6 +776,13 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
&EnvironmentOptions::trace_env_native_stack,
kAllowedInEnvvar);

AddOption(
"--trace-require-module",
"Print access to require(esm). Options are 'all' (print all usage) and "
"'no-node-modules' (excluding usage from the node_modules folder)",
&EnvironmentOptions::trace_require_module,
kAllowedInEnvvar);

AddOption("--extra-info-on-fatal-exception",
"hide extra information on fatal exception that causes exit",
&EnvironmentOptions::extra_info_on_fatal_exception,
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ class EnvironmentOptions : public Options {
bool trace_env = false;
bool trace_env_js_stack = false;
bool trace_env_native_stack = false;
std::string trace_require_module;
bool extra_info_on_fatal_exception = true;
std::string unhandled_rejections;
std::vector<std::string> userland_loaders;
Expand Down
11 changes: 0 additions & 11 deletions test/es-module/test-require-module-preload.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ const { spawnSyncAndAssert } = require('../common/child_process');
const { fixturesDir } = require('../common/fixtures');

function testPreload(preloadFlag) {
// The warning is only emitted when ESM is loaded by --require.
const isRequire = preloadFlag === '--require';
// Test named exports.
{
spawnSyncAndAssert(
Expand All @@ -22,8 +20,6 @@ function testPreload(preloadFlag) {
},
{
stdout: 'A',
stderr: isRequire ?
/ExperimentalWarning: --require is loading ES Module .*module-named-exports\.mjs using require/ : undefined,
trim: true,
}
);
Expand All @@ -43,8 +39,6 @@ function testPreload(preloadFlag) {
cwd: fixturesDir
},
{
stderr: isRequire ?
/ExperimentalWarning: --require is loading ES Module .*import-esm\.mjs using require/ : undefined,
stdout: /^world\s+A$/,
trim: true,
}
Expand All @@ -66,8 +60,6 @@ function testPreload(preloadFlag) {
},
{
stdout: /^ok\s+A$/,
stderr: isRequire ?
/ExperimentalWarning: --require is loading ES Module .*cjs-exports\.mjs using require/ : undefined,
trim: true,
}
);
Expand All @@ -90,8 +82,6 @@ function testPreload(preloadFlag) {
},
{
stdout: /^world\s+A$/,
stderr: isRequire ?
/ExperimentalWarning: --require is loading ES Module .*require-cjs\.mjs using require/ : undefined,
trim: true,
}
);
Expand All @@ -117,7 +107,6 @@ testPreload('--import');
},
{
stdout: /^package-type-module\s+A$/,
stderr: /ExperimentalWarning: --require is loading ES Module .*package-type-module[\\/]index\.js using require/,
trim: true,
}
);
Expand Down
14 changes: 11 additions & 3 deletions test/es-module/test-require-module-warning.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
'use strict';

// This checks the warning and the stack trace emitted by the require(esm)
// experimental warning. It can get removed when `require(esm)` becomes stable.

// This checks the warning and the stack trace emitted by --trace-require-module=all.
require('../common');
const { spawnSyncAndAssert } = require('../common/child_process');
const fixtures = require('../common/fixtures');
const assert = require('assert');

spawnSyncAndAssert(process.execPath, [
'--trace-warnings',
'--trace-require-module=all',
fixtures.path('es-modules', 'require-module.js'),
], {
trim: true,
Expand All @@ -33,3 +32,12 @@ spawnSyncAndAssert(process.execPath, [
);
}
});

spawnSyncAndAssert(process.execPath, [
'--trace-require-module=1',
fixtures.path('es-modules', 'require-module.js'),
], {
status: 9,
trim: true,
stderr: /invalid value for --trace-require-module/
});
10 changes: 0 additions & 10 deletions test/es-module/test-require-module.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,6 @@

const common = require('../common');
const assert = require('assert');
const path = require('path');

// Only the first load will trigger the warning.
common.expectWarning(
'ExperimentalWarning',
`CommonJS module ${__filename} is loading ES Module ` +
`${path.resolve(__dirname, '../fixtures/es-module-loaders/module-named-exports.mjs')} using require().\n` +
'Support for loading ES Module in require() is an experimental feature ' +
'and might change at any time'
);

// Test named exports.
{
Expand Down
24 changes: 18 additions & 6 deletions test/es-module/test-require-node-modules-warning.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';

// This checks the experimental warning for require(esm) is disabled when the
// require() comes from node_modules.
// This checks the warning and the stack trace emitted by
// --trace-require-module=no-node-modules.
require('../common');
const { spawnSyncAndAssert } = require('../common/child_process');
const fixtures = require('../common/fixtures');
Expand All @@ -14,7 +14,10 @@ const warningRE = /Support for loading ES Module in require\(\)/;
// require() in non-node_modules -> esm in node_modules should warn.
spawnSyncAndAssert(
process.execPath,
[fixtures.path('es-modules', 'test_node_modules', 'require-esm.js')],
[
'--trace-require-module=no-node-modules',
fixtures.path('es-modules', 'test_node_modules', 'require-esm.js'),
],
{
trim: true,
stderr: warningRE,
Expand All @@ -26,7 +29,10 @@ spawnSyncAndAssert(
// should not warn.
spawnSyncAndAssert(
process.execPath,
[fixtures.path('es-modules', 'test_node_modules', 'require-require-esm.js')],
[
'--trace-require-module=no-node-modules',
fixtures.path('es-modules', 'test_node_modules', 'require-require-esm.js'),
],
{
trim: true,
stderr: '',
Expand All @@ -38,7 +44,10 @@ spawnSyncAndAssert(
// should not warn.
spawnSyncAndAssert(
process.execPath,
[fixtures.path('es-modules', 'test_node_modules', 'import-require-esm.mjs')],
[
'--trace-require-module=no-node-modules',
fixtures.path('es-modules', 'test_node_modules', 'import-require-esm.mjs'),
],
{
trim: true,
stderr: '',
Expand All @@ -50,7 +59,10 @@ spawnSyncAndAssert(
// require() in node_modules -> esm in node_modules should not warn.
spawnSyncAndAssert(
process.execPath,
[fixtures.path('es-modules', 'test_node_modules', 'import-import-require-esm.mjs')],
[
'--trace-require-module=no-node-modules',
fixtures.path('es-modules', 'test_node_modules', 'import-import-require-esm.mjs'),
],
{
trim: true,
stderr: '',
Expand Down
1 change: 0 additions & 1 deletion test/es-module/test-typescript-commonjs.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ test('execute a .cts file importing a .mts file export', async () => {
fixtures.path('typescript/cts/test-require-mts-module.cts'),
]);

match(result.stderr, /Support for loading ES Module in require\(\) is an experimental feature and might change at any time/);
match(result.stdout, /Hello, TypeScript!/);
strictEqual(result.code, 0);
});
Expand Down
2 changes: 0 additions & 2 deletions test/es-module/test-typescript.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ test('expect failure of a TypeScript file requiring ES module syntax', async ()
fixtures.path('typescript/ts/test-require-module.ts'),
]);

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

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