module: unflag --experimental-require-module

This unflags --experimental-require-module so require(esm) can be used without the flag. For now, when require() actually encounters an ESM, it will still emit an experimental warning. To opt out of the feature, --no-experimental-require-module can be used. There are some tests specifically testing ERR_REQUIRE_ESM. Some of them are repurposed to test --no-experimental-require-module. Some of them are modified to just expect loading require(esm) to work, when it's appropriate. PR-URL: nodejs#55085 Refs: nodejs#52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
tpoisseau · Nov 21, 2024 · f73530a · f73530a
1 parent 2e50fc7
commit f73530a
Show file tree

Hide file tree

Showing 23 changed files with 135 additions and 68 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -1034,6 +1034,10 @@ following permissions are restricted:
 added:
   - v22.0.0
   - v20.17.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/55085
+    description: This is now true by default.
 -->
 
 > Stability: 1.1 - Active Development
@@ -1659,6 +1663,24 @@ added: v16.6.0
 
 Use this flag to disable top-level await in REPL.
 
+### `--no-experimental-require-module`
+
+<!-- YAML
+added:
+  - v22.0.0
+  - v20.17.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/55085
+    description: This is now false by default.
+-->
+
+> Stability: 1.1 - Active Development
+
+Disable support for loading a synchronous ES module graph in `require()`.
+
+See [Loading ECMAScript modules using `require()`][].
+
 ### `--no-experimental-websocket`
 
 <!-- YAML
@@ -1875,9 +1897,7 @@ added:
   - v20.17.0
 -->
 
-This flag is only useful when `--experimental-require-module` is enabled.
-
-If the ES module being `require()`'d contains top-level await, this flag
+If the ES module being `require()`'d contains top-level `await`, this flag
 allows Node.js to evaluate the module, try to locate the
 top-level awaits, and print their location to help users find them.
 

diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -2426,8 +2426,8 @@ object.
 
 > Stability: 1 - Experimental
 
-When trying to `require()` a [ES Module][] under `--experimental-require-module`,
-a CommonJS to ESM or ESM to CommonJS edge participates in an immediate cycle.
+When trying to `require()` a [ES Module][], a CommonJS to ESM or ESM to CommonJS edge
+participates in an immediate cycle.
 This is not allowed because ES Modules cannot be evaluated while they are
 already being evaluated.
 
@@ -2441,8 +2441,8 @@ module, and should be done lazily in an inner function.
 
 > Stability: 1 - Experimental
 
-When trying to `require()` a [ES Module][] under `--experimental-require-module`,
-the module turns out to be asynchronous. That is, it contains top-level await.
+When trying to `require()` a [ES Module][], the module turns out to be asynchronous.
+That is, it contains top-level await.
 
 To see where the top-level await is, use
 `--experimental-print-required-tla` (this would execute the modules
@@ -2452,12 +2452,20 @@ before looking for the top-level awaits).
 
 ### `ERR_REQUIRE_ESM`
 
-> Stability: 1 - Experimental
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/55085
+    description: require() now supports loading synchronous ES modules by default.
+-->
+
+> Stability: 0 - Deprecated
 
 An attempt was made to `require()` an [ES Module][].
 
-To enable `require()` for synchronous module graphs (without
-top-level `await`), use `--experimental-require-module`.
+This error has been deprecated since `require()` now supports loading synchronous
+ES modules. When `require()` encounters an ES module that contains top-level
+`await`, it will throw [`ERR_REQUIRE_ASYNC_MODULE`][] instead.
 
 <a id="ERR_SCRIPT_EXECUTION_INTERRUPTED"></a>
 
@@ -4061,6 +4069,7 @@ Type stripping is not supported for files descendent of a `node_modules` directo
 [`ERR_INVALID_ARG_TYPE`]: #err_invalid_arg_type
 [`ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST`]: #err_missing_message_port_in_transfer_list
 [`ERR_MISSING_TRANSFERABLE_IN_TRANSFER_LIST`]: #err_missing_transferable_in_transfer_list
+[`ERR_REQUIRE_ASYNC_MODULE`]: #err_require_async_module
 [`EventEmitter`]: events.md#class-eventemitter
 [`MessagePort`]: worker_threads.md#class-messageport
 [`Object.getPrototypeOf`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getPrototypeOf

diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -468,7 +468,7 @@ compatibility.
 ### `require`
 
 The CommonJS module `require` currently only supports loading synchronous ES
-modules when `--experimental-require-module` is enabled.
+modules (that is, ES modules that do not use top-level `await`).
 
 See [Loading ECMAScript modules using `require()`][] for details.
 

diff --git a/doc/api/modules.md b/doc/api/modules.md
@@ -170,15 +170,18 @@ relative, and based on the real path of the files making the calls to
 
 ## Loading ECMAScript modules using `require()`
 
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/55085
+    description: require() now supports loading synchronous ES modules by default.
+-->
+
 The `.mjs` extension is reserved for [ECMAScript Modules][].
-Currently, if the flag `--experimental-require-module` is not used, loading
-an ECMAScript module using `require()` will throw a [`ERR_REQUIRE_ESM`][]
-error, and users need to use [`import()`][] instead. See
-[Determining module system][] section for more info
+See [Determining module system][] section for more info
 regarding which files are parsed as ECMAScript modules.
 
-If `--experimental-require-module` is enabled, and the ECMAScript module being
-loaded by `require()` meets the following requirements:
+`require()` only supports loading ECMAScript modules that meet the following requirements:
 
 * The module is fully synchronous (contains no top-level `await`); and
 * One of these conditions are met:
@@ -187,8 +190,8 @@ loaded by `require()` meets the following requirements:
   3. The file has a `.js` extension, the closest `package.json` does not contain
      `"type": "commonjs"`, and the module contains ES module syntax.
 
-`require()` will load the requested module as an ES Module, and return
-the module namespace object. In this case it is similar to dynamic
+If the ES Module being loaded meet the requirements, `require()` can load it and
+return the module namespace object. In this case it is similar to dynamic
 `import()` but is run synchronously and returns the name space object
 directly.
 
@@ -207,7 +210,7 @@ class Point {
 export default Point;
 ```
 
-A CommonJS module can load them with `require()` under `--experimental-detect-module`:
+A CommonJS module can load them with `require()`:
 
 ```cjs
 const distance = require('./distance.mjs');
@@ -236,13 +239,19 @@ conventions. Code authored directly in CommonJS should avoid depending on it.
 If the module being `require()`'d contains top-level `await`, or the module
 graph it `import`s contains top-level `await`,
 [`ERR_REQUIRE_ASYNC_MODULE`][] will be thrown. In this case, users should
-load the asynchronous module using `import()`.
+load the asynchronous module using [`import()`][].
 
 If `--experimental-print-required-tla` is enabled, instead of throwing
 `ERR_REQUIRE_ASYNC_MODULE` before evaluation, Node.js will evaluate the
 module, try to locate the top-level awaits, and print their location to
 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.
+
 ## All together
 
 <!-- type=misc -->
@@ -272,8 +281,7 @@ require(X) from module at path Y
 
 MAYBE_DETECT_AND_LOAD(X)
 1. If X parses as a CommonJS module, load X as a CommonJS module. STOP.
-2. Else, if `--experimental-require-module` is
-  enabled, and the source code of X can be parsed as ECMAScript module using
+2. Else, if the source code of X can be parsed as ECMAScript module using
   <a href="esm.md#resolver-algorithm-specification">DETECT_MODULE_SYNTAX defined in
   the ESM resolver</a>,
   a. Load X as an ECMAScript module. STOP.
@@ -1190,7 +1198,6 @@ This section was moved to
 [`"main"`]: packages.md#main
 [`"type"`]: packages.md#type
 [`ERR_REQUIRE_ASYNC_MODULE`]: errors.md#err_require_async_module
-[`ERR_REQUIRE_ESM`]: errors.md#err_require_esm
 [`ERR_UNSUPPORTED_DIR_IMPORT`]: errors.md#err_unsupported_dir_import
 [`MODULE_NOT_FOUND`]: errors.md#module_not_found
 [`__dirname`]: #__dirname

diff --git a/doc/api/packages.md b/doc/api/packages.md
@@ -172,8 +172,7 @@ There is the CommonJS module loader:
 * It treats all files that lack `.json` or `.node` extensions as JavaScript
   text files.
 * It can only be used to [load ECMASCript modules from CommonJS modules][] if
-  the module graph is synchronous (that contains no top-level `await`) when
-  `--experimental-require-module` is enabled.
+  the module graph is synchronous (that contains no top-level `await`).
   When used to load a JavaScript text file that is not an ECMAScript module,
   the file will be loaded as a CommonJS module.
 
@@ -662,8 +661,7 @@ specific to least specific as conditions should be defined:
 * `"require"` - matches when the package is loaded via `require()`. The
   referenced file should be loadable with `require()` although the condition
   matches regardless of the module format of the target file. Expected
-  formats include CommonJS, JSON, native addons, and ES modules
-  if `--experimental-require-module` is enabled. _Always mutually
+  formats include CommonJS, JSON, native addons, and ES modules. _Always mutually
   exclusive with `"import"`._
 * `"module-sync"` - matches no matter the package is loaded via `import`,
   `import()` or `require()`. The format is expected to be ES modules that does

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -438,7 +438,6 @@ function initializeCJS() {
     Module._extensions['.ts'] = loadTS;
   }
   if (getOptionValue('--experimental-require-module')) {
-    emitExperimentalWarning('Support for loading ES Module in require()');
     Module._extensions['.mjs'] = loadESMFromCJS;
     if (tsEnabled) {
       Module._extensions['.mts'] = loadESMFromCJS;
@@ -1386,6 +1385,7 @@ function loadESMFromCJS(mod, filename) {
     // ESM won't be accessible via process.mainModule.
     setOwnProperty(process, 'mainModule', undefined);
   } else {
+    emitExperimentalWarning('Support for loading ES Module in require()');
     const {
       wrap,
       namespace,

diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js
@@ -131,11 +131,6 @@ async function defaultLoad(url, context = kEmptyObject) {
 
   validateAttributes(url, format, importAttributes);
 
-  // Use the synchronous commonjs translator which can deal with cycles.
-  if (format === 'commonjs' && getOptionValue('--experimental-require-module')) {
-    format = 'commonjs-sync';
-  }
-
   if (getOptionValue('--experimental-strip-types') &&
     (format === 'module-typescript' || format === 'commonjs-typescript') &&
     isUnderNodeModules(url)) {
@@ -191,11 +186,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
 
   validateAttributes(url, format, importAttributes);
 
-  // Use the synchronous commonjs translator which can deal with cycles.
-  if (format === 'commonjs' && getOptionValue('--experimental-require-module')) {
-    format = 'commonjs-sync';
-  }
-
   return {
     __proto__: null,
     format,

diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -373,15 +373,15 @@ class ModuleLoader {
 
     defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync;
     const loadResult = defaultLoadSync(url, { format, importAttributes });
-    const {
-      format: finalFormat,
-      source,
-    } = loadResult;
+
+    // Use the synchronous commonjs translator which can deal with cycles.
+    const finalFormat = loadResult.format === 'commonjs' ? 'commonjs-sync' : loadResult.format;
 
     if (finalFormat === 'wasm') {
       assert.fail('WASM is currently unsupported by require(esm)');
     }
 
+    const { source } = loadResult;
     const isMain = (parentURL === undefined);
     const wrap = this.#translate(url, finalFormat, source, isMain);
     assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);

diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
@@ -361,6 +361,7 @@ class ModuleJobSync extends ModuleJobBase {
   }
 
   runSync() {
+    // TODO(joyeecheung): add the error decoration logic from the async instantiate.
     this.module.instantiateSync();
     setHasStartedUserESMExecution();
     const namespace = this.module.evaluateSync();

diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js
@@ -75,17 +75,15 @@ function initializeDefaultConditions() {
   const userConditions = getOptionValue('--conditions');
   const noAddons = getOptionValue('--no-addons');
   const addonConditions = noAddons ? [] : ['node-addons'];
-
+  const moduleConditions = getOptionValue('--experimental-require-module') ? ['module-sync'] : [];
   defaultConditions = ObjectFreeze([
     'node',
     'import',
+    ...moduleConditions,
     ...addonConditions,
     ...userConditions,
   ]);
   defaultConditionsSet = new SafeSet(defaultConditions);
-  if (getOptionValue('--experimental-require-module')) {
-    defaultConditionsSet.add('module-sync');
-  }
 }
 
 /**

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -374,9 +374,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             &EnvironmentOptions::print_required_tla,
             kAllowedInEnvvar);
   AddOption("--experimental-require-module",
-            "Allow loading explicit ES Modules in require().",
+            "Allow loading synchronous ES Modules in require().",
             &EnvironmentOptions::require_module,
-            kAllowedInEnvvar);
+            kAllowedInEnvvar,
+            true);
   AddOption("--diagnostic-dir",
             "set dir for all output files"
             " (default: current working directory)",

diff --git a/src/node_options.h b/src/node_options.h
@@ -117,7 +117,7 @@ class EnvironmentOptions : public Options {
   std::vector<std::string> conditions;
   bool detect_module = true;
   bool print_required_tla = false;
-  bool require_module = false;
+  bool require_module = true;
   std::string dns_result_order;
   bool enable_source_maps = false;
   bool experimental_eventsource = false;

diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js
@@ -1,3 +1,6 @@
+// Previously, this tested that require(esm) throws ERR_REQUIRE_ESM, which is no longer applicable
+// since require(esm) is now supported. The test has been repurposed to ensure that the old behavior
+// is preserved when the --no-experimental-require-module flag is used.
 'use strict';
 
 const { spawnPromisified } = require('../common');
@@ -22,7 +25,9 @@ describe('CJS ↔︎ ESM interop warnings', { concurrency: !process.env.TEST_PAR
       fixtures.path('/es-modules/package-type-module/cjs.js')
     );
     const basename = 'cjs.js';
-    const { code, signal, stderr } = await spawnPromisified(execPath, [requiringCjsAsEsm]);
+    const { code, signal, stderr } = await spawnPromisified(execPath, [
+      '--no-experimental-require-module', requiringCjsAsEsm,
+    ]);
 
     assert.ok(
       stderr.replaceAll('\r', '').includes(
@@ -48,7 +53,9 @@ describe('CJS ↔︎ ESM interop warnings', { concurrency: !process.env.TEST_PAR
       fixtures.path('/es-modules/package-type-module/esm.js')
     );
     const basename = 'esm.js';
-    const { code, signal, stderr } = await spawnPromisified(execPath, [requiringEsm]);
+    const { code, signal, stderr } = await spawnPromisified(execPath, [
+      '--no-experimental-require-module', requiringEsm,
+    ]);
 
     assert.ok(
       stderr.replace(/\r/g, '').includes(

diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs
@@ -402,15 +402,16 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
   });
 });
 
-// Validate temporarily disabling `--abort-on-uncaught-exception`
-// while running `containsModuleSyntax`.
+// Checks the error caught during module detection does not trigger abort when
+// `--abort-on-uncaught-exception` is passed in (as that's a caught internal error).
 // Ref: https://github.com/nodejs/node/issues/50878
 describe('Wrapping a `require` of an ES module while using `--abort-on-uncaught-exception`', () => {
   it('should work', async () => {
     const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
       '--abort-on-uncaught-exception',
+      '--no-warnings',
       '--eval',
-      'assert.throws(() => require("./package-type-module/esm.js"), { code: "ERR_REQUIRE_ESM" })',
+      'require("./package-type-module/esm.js")',
     ], {
       cwd: fixtures.path('es-modules'),
     });

diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs
@@ -774,6 +774,7 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => {
 
   describe('should use hooks', async () => {
     const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+      '--no-experimental-require-module',
       '--import',
       fixtures.fileURL('es-module-loaders/builtin-named-exports.mjs'),
       fixtures.path('es-modules/require-esm-throws-with-loaders.js'),

diff --git a/test/es-module/test-esm-type-field-errors-2.js b/test/es-module/test-esm-type-field-errors-2.js
@@ -0,0 +1,17 @@
+// Flags: --no-experimental-require-module
+// Previously, this tested that require(esm) throws ERR_REQUIRE_ESM, which is no longer applicable
+// since require(esm) is now supported. The test has been repurposed to ensure that the old behavior
+// is preserved when the --no-experimental-require-module flag is used.
+
+'use strict';
+require('../common');
+const assert = require('assert');
+const { describe, it } = require('node:test');
+
+describe('Errors related to ESM type field', () => {
+  it('Should throw an error when loading CJS from a `type: "module"` package.', () => {
+    assert.throws(() => require('../fixtures/es-modules/package-type-module/index.js'), {
+      code: 'ERR_REQUIRE_ESM'
+    });
+  });
+});