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.
nodejs · Sep 24, 2024 · 7af5b5d · 7af5b5d
1 parent 3c5ceff
commit 7af5b5d
Show file tree

Hide file tree

Showing 15 changed files with 67 additions and 34 deletions.
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -437,7 +437,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
@@ -372,15 +372,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/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'
+    });
+  });
+});
diff --git a/test/es-module/test-esm-type-field-errors.js b/test/es-module/test-esm-type-field-errors.js
@@ -50,12 +50,6 @@ describe('ESM type field errors', { concurrency: true }, () => {
       true,
     );
   });
-
-  it('--input-type=module disallowed for directories', () => {
-    assert.throws(() => require('../fixtures/es-modules/package-type-module/index.js'), {
-      code: 'ERR_REQUIRE_ESM'
-    });
-  });
 });
 
 function expect(opt = '', inputFile, want, wantsError = false) {

diff --git a/test/es-module/test-require-module-preload.js b/test/es-module/test-require-module-preload.js
@@ -3,9 +3,12 @@
 require('../common');
 const { spawnSyncAndAssert } = require('../common/child_process');
 const { fixturesDir } = require('../common/fixtures');
-const stderr = /ExperimentalWarning: Support for loading ES Module in require/;
 
+const warningRE = /ExperimentalWarning: Support for loading ES Module in require/;
 function testPreload(preloadFlag) {
+  // The warning is only emitted when ESM is loaded by --require.
+  const stderr = preloadFlag !== '--import' ? warningRE : undefined;
+
   // Test named exports.
   {
     spawnSyncAndAssert(
@@ -112,7 +115,7 @@ testPreload('--import');
     },
     {
       stdout: /^package-type-module\s+A$/,
-      stderr,
+      stderr: warningRE,
       trim: true,
     }
   );

diff --git a/test/es-module/test-typescript-commonjs.mjs b/test/es-module/test-typescript-commonjs.mjs
@@ -100,6 +100,7 @@ test('execute a .cts file importing a .ts file export', async () => {
 test('execute a .cts file importing a .mts file export', async () => {
   const result = await spawnPromisified(process.execPath, [
     '--experimental-strip-types',
+    '--no-experimental-require-module',
     fixtures.path('typescript/cts/test-require-mts-module.cts'),
   ]);
 
@@ -158,6 +159,7 @@ test('expect failure of a .ts file in node_modules', async () => {
 test('expect failure of a .cts requiring esm without default type module', async () => {
   const result = await spawnPromisified(process.execPath, [
     '--experimental-strip-types',
+    '--no-experimental-require-module',
     fixtures.path('typescript/cts/test-mts-node_modules.cts'),
   ]);
 

diff --git a/test/es-module/test-typescript-module.mjs b/test/es-module/test-typescript-module.mjs
@@ -67,6 +67,7 @@ test('execute an .mts file importing a .cts file', async () => {
 test('execute an .mts file with wrong default module', async () => {
   const result = await spawnPromisified(process.execPath, [
     '--experimental-strip-types',
+    '--no-experimental-require-module',
     '--experimental-default-type=commonjs',
     fixtures.path('typescript/mts/test-import-module.mts'),
   ]);
@@ -76,6 +77,18 @@ test('execute an .mts file with wrong default module', async () => {
   strictEqual(result.code, 1);
 });
 
+test('execute an .mts file with wrong default module', async () => {
+  const result = await spawnPromisified(process.execPath, [
+    '--experimental-strip-types',
+    '--experimental-require-module',
+    '--experimental-default-type=commonjs',
+    fixtures.path('typescript/mts/test-import-module.mts'),
+  ]);
+
+  match(result.stdout, /Hello, TypeScript!/);
+  strictEqual(result.code, 0);
+});
+
 test('execute an .mts file from node_modules', async () => {
   const result = await spawnPromisified(process.execPath, [
     '--experimental-strip-types',

diff --git a/test/es-module/test-typescript.mjs b/test/es-module/test-typescript.mjs
@@ -32,11 +32,11 @@ test('execute a TypeScript file with imports', async () => {
   const result = await spawnPromisified(process.execPath, [
     '--no-warnings',
     '--eval',
-    `assert.throws(() => require(${JSON.stringify(fixtures.path('typescript/ts/test-import-fs.ts'))}), {code: 'ERR_REQUIRE_ESM'})`,
+    `require(${JSON.stringify(fixtures.path('typescript/ts/test-import-fs.ts'))})`,
   ]);
 
   strictEqual(result.stderr, '');
-  strictEqual(result.stdout, '');
+  match(result.stdout, /Hello, TypeScript!/);
   strictEqual(result.code, 0);
 });
 
@@ -283,9 +283,8 @@ test('execute a TypeScript file with CommonJS syntax requiring .mts', async () =
     fixtures.path('typescript/ts/test-require-mts.ts'),
   ]);
 
-  strictEqual(result.stdout, '');
-  match(result.stderr, /Error \[ERR_REQUIRE_ESM\]: require\(\) of ES Module/);
-  strictEqual(result.code, 1);
+  match(result.stdout, /Hello, TypeScript!/);
+  strictEqual(result.code, 0);
 });
 
 test('execute a TypeScript file with CommonJS syntax requiring .mts with require-module', async () => {

diff --git a/test/parallel/test-require-mjs.js b/test/parallel/test-require-mjs.js
@@ -1,3 +1,8 @@
+// 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');