esm: bypass CommonJS loader under --default-type

PR-URL: nodejs#49986 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos · Nov 11, 2023 · 151e812 · 151e812
1 parent 08201aa
commit 151e812
Show file tree

Hide file tree

Showing 8 changed files with 179 additions and 41 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -25,14 +25,16 @@ For more info about `node inspect`, see the [debugger][] documentation.
 
 The program entry point is a specifier-like string. If the string is not an
 absolute path, it's resolved as a relative path from the current working
-directory. That path is then resolved by [CommonJS][] module loader. If no
-corresponding file is found, an error is thrown.
+directory. That path is then resolved by [CommonJS][] module loader, or by the
+[ES module loader][Modules loaders] if [`--experimental-default-type=module`][]
+is passed. If no corresponding file is found, an error is thrown.
 
 If a file is found, its path will be passed to the
 [ES module loader][Modules loaders] under any of the following conditions:
 
 * The program was started with a command-line flag that forces the entry
-  point to be loaded with ECMAScript module loader.
+  point to be loaded with ECMAScript module loader, such as `--import` or
+  [`--experimental-default-type=module`][].
 * The file has an `.mjs` extension.
 * The file does not have a `.cjs` extension, and the nearest parent
   `package.json` file contains a top-level [`"type"`][] field with a value of
@@ -45,8 +47,9 @@ Otherwise, the file is loaded using the CommonJS module loader. See
 
 When loading, the [ES module loader][Modules loaders] loads the program
 entry point, the `node` command will accept as input only files with `.js`,
-`.mjs`, or `.cjs` extensions; and with `.wasm` extensions when
-[`--experimental-wasm-modules`][] is enabled.
+`.mjs`, or `.cjs` extensions; with `.wasm` extensions when
+[`--experimental-wasm-modules`][] is enabled; and with no extension when
+[`--experimental-default-type=module`][] is passed.
 
 ## Options
 
@@ -2417,6 +2420,7 @@ done
 [`"type"`]: packages.md#type
 [`--cpu-prof-dir`]: #--cpu-prof-dir
 [`--diagnostic-dir`]: #--diagnostic-dirdirectory
+[`--experimental-default-type=module`]: #--experimental-default-typetype
 [`--experimental-wasm-modules`]: #--experimental-wasm-modules
 [`--heap-prof-dir`]: #--heap-prof-dir
 [`--import`]: #--importmodule

diff --git a/lib/internal/main/run_main_module.js b/lib/internal/main/run_main_module.js
@@ -6,18 +6,24 @@ const {
   prepareMainThreadExecution,
   markBootstrapComplete,
 } = require('internal/process/pre_execution');
+const { getOptionValue } = require('internal/options');
 
-prepareMainThreadExecution(true);
+const mainEntry = prepareMainThreadExecution(true);
 
 markBootstrapComplete();
 
 // Necessary to reset RegExp statics before user code runs.
 RegExpPrototypeExec(/^/, '');
 
-// Note: this loads the module through the ESM loader if the module is
-// determined to be an ES module. This hangs from the CJS module loader
-// because we currently allow monkey-patching of the module loaders
-// in the preloaded scripts through require('module').
-// runMain here might be monkey-patched by users in --require.
-// XXX: the monkey-patchability here should probably be deprecated.
-require('internal/modules/cjs/loader').Module.runMain(process.argv[1]);
+if (getOptionValue('--experimental-default-type') === 'module') {
+  require('internal/modules/run_main').executeUserEntryPoint(mainEntry);
+} else {
+  /**
+   * To support legacy monkey-patching of `Module.runMain`, we call `runMain` here to have the CommonJS loader begin
+   * the execution of the main entry point, even if the ESM loader immediately takes over because the main entry is an
+   * ES module or one of the other opt-in conditions (such as the use of `--import`) are met. Users can monkey-patch
+   * before the main entry point is loaded by doing so via scripts loaded through `--require`. This monkey-patchability
+   * is undesirable and is removed in `--experimental-default-type=module` mode.
+   */
+  require('internal/modules/cjs/loader').Module.runMain(mainEntry);
+}
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -1204,17 +1204,7 @@ function defaultResolve(specifier, context = {}) {
       if (StringPrototypeStartsWith(specifier, 'file://')) {
         specifier = fileURLToPath(specifier);
       }
-      const found = resolveAsCommonJS(specifier, parentURL);
-      if (found) {
-        // Modify the stack and message string to include the hint
-        const lines = StringPrototypeSplit(error.stack, '\n');
-        const hint = `Did you mean to import ${found}?`;
-        error.stack =
-          ArrayPrototypeShift(lines) + '\n' +
-          hint + '\n' +
-          ArrayPrototypeJoin(lines, '\n');
-        error.message += `\n${hint}`;
-      }
+      decorateErrorWithCommonJSHints(error, specifier, parentURL);
     }
     throw error;
   }
@@ -1227,7 +1217,28 @@ function defaultResolve(specifier, context = {}) {
   };
 }
 
+/**
+ * Decorates the given error with a hint for CommonJS modules.
+ * @param {Error} error - The error to decorate.
+ * @param {string} specifier - The specifier that was attempted to be imported.
+ * @param {string} parentURL - The URL of the parent module.
+ */
+function decorateErrorWithCommonJSHints(error, specifier, parentURL) {
+  const found = resolveAsCommonJS(specifier, parentURL);
+  if (found) {
+    // Modify the stack and message string to include the hint
+    const lines = StringPrototypeSplit(error.stack, '\n');
+    const hint = `Did you mean to import ${found}?`;
+    error.stack =
+      ArrayPrototypeShift(lines) + '\n' +
+      hint + '\n' +
+      ArrayPrototypeJoin(lines, '\n');
+    error.message += `\n${hint}`;
+  }
+}
+
 module.exports = {
+  decorateErrorWithCommonJSHints,
   defaultResolve,
   encodedSepRegEx,
   getPackageScopeConfig,

diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js
@@ -1,7 +1,6 @@
 'use strict';
 
 const {
-  ObjectCreate,
   StringPrototypeEndsWith,
 } = primordials;
 
@@ -13,17 +12,33 @@ const path = require('path');
  * @param {string} main - Entry point path
  */
 function resolveMainPath(main) {
-  // Note extension resolution for the main entry point can be deprecated in a
-  // future major.
-  // Module._findPath is monkey-patchable here.
-  const { Module } = require('internal/modules/cjs/loader');
-  let mainPath = Module._findPath(path.resolve(main), null, true);
+  const defaultType = getOptionValue('--experimental-default-type');
+  /** @type {string} */
+  let mainPath;
+  if (defaultType === 'module') {
+    if (getOptionValue('--preserve-symlinks-main')) { return; }
+    mainPath = path.resolve(main);
+  } else {
+    // Extension searching for the main entry point is supported only in legacy mode.
+    // Module._findPath is monkey-patchable here.
+    const { Module } = require('internal/modules/cjs/loader');
+    mainPath = Module._findPath(path.resolve(main), null, true);
+  }
   if (!mainPath) { return; }
 
   const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
   if (!preserveSymlinksMain) {
     const { toRealPath } = require('internal/modules/helpers');
-    mainPath = toRealPath(mainPath);
+    try {
+      mainPath = toRealPath(mainPath);
+    } catch (err) {
+      if (defaultType === 'module' && err?.code === 'ENOENT') {
+        const { decorateErrorWithCommonJSHints } = require('internal/modules/esm/resolve');
+        const { getCWDURL } = require('internal/util');
+        decorateErrorWithCommonJSHints(err, mainPath, getCWDURL());
+      }
+      throw err;
+    }
   }
 
   return mainPath;
@@ -34,6 +49,8 @@ function resolveMainPath(main) {
  * @param {string} mainPath - Absolute path to the main entry point
  */
 function shouldUseESMLoader(mainPath) {
+  if (getOptionValue('--experimental-default-type') === 'module') { return true; }
+
   /**
    * @type {string[]} userLoaders A list of custom loaders registered by the user
    * (or an empty list when none have been registered).
@@ -69,11 +86,10 @@ function shouldUseESMLoader(mainPath) {
 function runMainESM(mainPath) {
   const { loadESM } = require('internal/process/esm_loader');
   const { pathToFileURL } = require('internal/url');
+  const main = pathToFileURL(mainPath).href;
 
   handleMainPromise(loadESM((esmLoader) => {
-    const main = path.isAbsolute(mainPath) ?
-      pathToFileURL(mainPath).href : mainPath;
-    return esmLoader.import(main, undefined, ObjectCreate(null));
+    return esmLoader.import(main, undefined, { __proto__: null });
   }));
 }
 
@@ -97,8 +113,9 @@ async function handleMainPromise(promise) {
  * Parse the CLI main entry point string and run it.
  * For backwards compatibility, we have to run a bunch of monkey-patchable code that belongs to the CJS loader (exposed
  * by `require('module')`) even when the entry point is ESM.
+ * This monkey-patchable code is bypassed under `--experimental-default-type=module`.
  * Because of backwards compatibility, this function is exposed publicly via `import { runMain } from 'node:module'`.
- * @param {string} main - Resolved absolute path for the main entry point, if found
+ * @param {string} main - First positional CLI argument, such as `'entry.js'` from `node entry.js`
  */
 function executeUserEntryPoint(main = process.argv[1]) {
   const resolvedMain = resolveMainPath(main);

diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js
@@ -37,7 +37,7 @@ const {
 } = require('internal/v8/startup_snapshot');
 
 function prepareMainThreadExecution(expandArgv1 = false, initializeModules = true) {
-  prepareExecution({
+  return prepareExecution({
     expandArgv1,
     initializeModules,
     isMainThread: true,
@@ -58,8 +58,8 @@ function prepareExecution(options) {
   refreshRuntimeOptions();
   reconnectZeroFillToggle();
 
-  // Patch the process object with legacy properties and normalizations
-  patchProcessObject(expandArgv1);
+  // Patch the process object and get the resolved main entry point.
+  const mainEntry = patchProcessObject(expandArgv1);
   setupTraceCategoryState();
   setupPerfHooks();
   setupInspectorHooks();
@@ -113,6 +113,8 @@ function prepareExecution(options) {
   if (initializeModules) {
     setupUserModules();
   }
+
+  return mainEntry;
 }
 
 function setupSymbolDisposePolyfill() {
@@ -167,14 +169,17 @@ function patchProcessObject(expandArgv1) {
   process._exiting = false;
   process.argv[0] = process.execPath;
 
+  /** @type {string} */
+  let mainEntry;
   // If requested, update process.argv[1] to replace whatever the user provided with the resolved absolute file path of
   // the entry point.
   if (expandArgv1 && process.argv[1] &&
       !StringPrototypeStartsWith(process.argv[1], '-')) {
     // Expand process.argv[1] into a full path.
     const path = require('path');
     try {
-      process.argv[1] = path.resolve(process.argv[1]);
+      mainEntry = path.resolve(process.argv[1]);
+      process.argv[1] = mainEntry;
     } catch {
       // Continue regardless of error.
     }
@@ -201,6 +206,8 @@ function patchProcessObject(expandArgv1) {
   addReadOnlyProcessAlias('traceDeprecation', '--trace-deprecation');
   addReadOnlyProcessAlias('_breakFirstLine', '--inspect-brk', false);
   addReadOnlyProcessAlias('_breakNodeFirstLine', '--inspect-brk-node', false);
+
+  return mainEntry;
 }
 
 function addReadOnlyProcessAlias(name, option, enumerable = true) {

diff --git a/test/es-module/test-esm-type-flag-cli-entry.mjs b/test/es-module/test-esm-type-flag-cli-entry.mjs
@@ -0,0 +1,92 @@
+import { spawnPromisified } from '../common/index.mjs';
+import * as fixtures from '../common/fixtures.mjs';
+import { describe, it } from 'node:test';
+import { match, strictEqual } from 'node:assert';
+
+describe('--experimental-default-type=module should not support extension searching', { concurrency: true }, () => {
+  it('should support extension searching under --experimental-default-type=commonjs', async () => {
+    const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+      '--experimental-default-type=commonjs',
+      'index',
+    ], {
+      cwd: fixtures.path('es-modules/package-without-type'),
+    });
+
+    strictEqual(stdout, 'package-without-type\n');
+    strictEqual(stderr, '');
+    strictEqual(code, 0);
+    strictEqual(signal, null);
+  });
+
+  it('should error with implicit extension under --experimental-default-type=module', async () => {
+    const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+      '--experimental-default-type=module',
+      'index',
+    ], {
+      cwd: fixtures.path('es-modules/package-without-type'),
+    });
+
+    match(stderr, /ENOENT.*Did you mean to import .*index\.js\?/s);
+    strictEqual(stdout, '');
+    strictEqual(code, 1);
+    strictEqual(signal, null);
+  });
+});
+
+describe('--experimental-default-type=module should not parse paths as URLs', { concurrency: true }, () => {
+  it('should not parse a `?` in a filename as starting a query string', async () => {
+    const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+      '--experimental-default-type=module',
+      'file#1.js',
+    ], {
+      cwd: fixtures.path('es-modules/package-without-type'),
+    });
+
+    strictEqual(stderr, '');
+    strictEqual(stdout, 'file#1\n');
+    strictEqual(code, 0);
+    strictEqual(signal, null);
+  });
+
+  it('should resolve `..`', async () => {
+    const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+      '--experimental-default-type=module',
+      '../package-without-type/file#1.js',
+    ], {
+      cwd: fixtures.path('es-modules/package-without-type'),
+    });
+
+    strictEqual(stderr, '');
+    strictEqual(stdout, 'file#1\n');
+    strictEqual(code, 0);
+    strictEqual(signal, null);
+  });
+
+  it('should allow a leading `./`', async () => {
+    const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+      '--experimental-default-type=module',
+      './file#1.js',
+    ], {
+      cwd: fixtures.path('es-modules/package-without-type'),
+    });
+
+    strictEqual(stderr, '');
+    strictEqual(stdout, 'file#1\n');
+    strictEqual(code, 0);
+    strictEqual(signal, null);
+  });
+
+  it('should not require a leading `./`', async () => {
+    const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+      '--experimental-default-type=module',
+      'file#1.js',
+    ], {
+      cwd: fixtures.path('es-modules/package-without-type'),
+    });
+
+    strictEqual(stderr, '');
+    strictEqual(stdout, 'file#1\n');
+    strictEqual(code, 0);
+    strictEqual(signal, null);
+  });
+});
diff --git a/test/fixtures/errors/force_colors.snapshot b/test/fixtures/errors/force_colors.snapshot
@@ -8,7 +8,7 @@ Error: Should include grayed stack trace
 [90m    at Module._extensions..js (node:internal*modules*cjs*loader:1414:10)[39m
 [90m    at Module.load (node:internal*modules*cjs*loader:1197:32)[39m
 [90m    at Module._load (node:internal*modules*cjs*loader:1013:12)[39m
-[90m    at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:109:12)[39m
-[90m    at node:internal*main*run_main_module:23:47[39m
+[90m    at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:128:12)[39m
+[90m    at node:internal*main*run_main_module:28:49[39m
 
 Node.js *
diff --git a/test/fixtures/es-modules/package-without-type/file#1.js b/test/fixtures/es-modules/package-without-type/file#1.js
@@ -0,0 +1 @@
+console.log('file#1');