From 7987ae59e46acb03d488595256d8609edbb83fa0 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Mon, 9 Dec 2024 15:47:25 +0100
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/56194
Fixes: https://github.com/nodejs/node/issues/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>
---
 doc/api/cli.md                                | 13 +++++++++
 doc/api/modules.md                            | 15 +++++++---
 lib/internal/modules/cjs/loader.js            | 29 +++++++++++--------
 src/node_options.cc                           | 13 +++++++++
 src/node_options.h                            |  1 +
 test/es-module/test-require-module-preload.js | 11 -------
 test/es-module/test-require-module-warning.js | 14 +++++++--
 test/es-module/test-require-module.js         | 10 -------
 .../test-require-node-modules-warning.js      | 24 +++++++++++----
 9 files changed, 84 insertions(+), 46 deletions(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index 88c961b99a0a42f..2fd8a5bc01202ff 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -2387,6 +2387,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
@@ -2865,6 +2877,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`
diff --git a/doc/api/modules.md b/doc/api/modules.md
index 9e4d3a223b8df60..fb92222a2b19a7f 100644
--- a/doc/api/modules.md
+++ b/doc/api/modules.md
@@ -175,7 +175,14 @@ added:
   - v22.0.0
   - v20.17.0
 changes:
-  - version: REPLACEME
+  - 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
     pr-url: https://github.com/nodejs/node/pull/55085
     description: This feature is no longer behind the `--experimental-require-module` CLI flag.
   - version: REPLACEME
@@ -315,9 +322,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`.
 
@@ -1266,6 +1272,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
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index ba80ee9ee5c8aa2..c30be425b6b9e61 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -1349,7 +1349,7 @@ Module.prototype.require = function(id) {
   }
 };
 
-let emittedRequireModuleWarning = false;
+let requireModuleWarningMode;
 /**
  * Resolved path to `process.argv[1]` will be lazily placed here
  * (needed for setting breakpoint when called with `--inspect-brk`).
@@ -1378,17 +1378,22 @@ function loadESMFromCJS(mod, filename) {
   } 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;
@@ -1414,7 +1419,7 @@ function loadESMFromCJS(mod, filename) {
                                 messagePrefix,
                                 undefined,
                                 parent?.require);
-        emittedRequireModuleWarning = true;
+        requireModuleWarningMode = true;
       }
     }
     const {
diff --git a/src/node_options.cc b/src/node_options.cc
index e6924b29d0b49e6..dd4af1049c8925d 100644
--- a/src/node_options.cc
+++ b/src/node_options.cc
@@ -150,6 +150,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 (syntax_check_only) {
       errors->push_back("either --test or --check can be used, not both");
@@ -734,6 +739,14 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "set module system to use by default",
             &EnvironmentOptions::type,
             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,
diff --git a/src/node_options.h b/src/node_options.h
index 6f95470c7f513d3..1d83885fbfa1378 100644
--- a/src/node_options.h
+++ b/src/node_options.h
@@ -199,6 +199,7 @@ class EnvironmentOptions : public Options {
   bool trace_uncaught = false;
   bool trace_warnings = false;
   bool trace_promises = false;
+  std::string trace_require_module;
   bool extra_info_on_fatal_exception = true;
   std::string unhandled_rejections;
   std::vector<std::string> userland_loaders;
diff --git a/test/es-module/test-require-module-preload.js b/test/es-module/test-require-module-preload.js
index 7a8a09486b43d76..0b8b79118ee8e6f 100644
--- a/test/es-module/test-require-module-preload.js
+++ b/test/es-module/test-require-module-preload.js
@@ -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(
@@ -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,
       }
     );
@@ -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,
       }
@@ -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,
       }
     );
@@ -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,
       }
     );
@@ -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,
     }
   );
diff --git a/test/es-module/test-require-module-warning.js b/test/es-module/test-require-module-warning.js
index d5be2fc6da87552..4c651efd5c0cc75 100644
--- a/test/es-module/test-require-module-warning.js
+++ b/test/es-module/test-require-module-warning.js
@@ -1,8 +1,6 @@
 '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');
@@ -10,6 +8,7 @@ const assert = require('assert');
 
 spawnSyncAndAssert(process.execPath, [
   '--trace-warnings',
+  '--trace-require-module=all',
   fixtures.path('es-modules', 'require-module.js'),
 ], {
   trim: true,
@@ -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/
+});
diff --git a/test/es-module/test-require-module.js b/test/es-module/test-require-module.js
index 4987fbf7b07d434..7e05fbd4a5bf88d 100644
--- a/test/es-module/test-require-module.js
+++ b/test/es-module/test-require-module.js
@@ -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.
 {
diff --git a/test/es-module/test-require-node-modules-warning.js b/test/es-module/test-require-node-modules-warning.js
index 837f174fd289500..bb3e38b7d92071d 100644
--- a/test/es-module/test-require-node-modules-warning.js
+++ b/test/es-module/test-require-node-modules-warning.js
@@ -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');
@@ -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,
@@ -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: '',
@@ -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: '',
@@ -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: '',