module: resolve format for all situations with module detection on

Author: dygabo

lib/internal/modules/esm/get_format.js

@@ -22,6 +22,8 @@ const experimentalNetworkImports =
 const { containsModuleSyntax } = internalBinding('contextify');
 const { getPackageScopeConfig, getPackageType } = require('internal/modules/package_json_reader');
 const { fileURLToPath } = require('internal/url');
+const { readFileSync } = require('fs');
+const { Buffer } = require('buffer');
 const { ERR_UNKNOWN_FILE_EXTENSION } = require('internal/errors').codes;
 
 const protocolHandlers = {
@@ -82,6 +84,24 @@ function underNodeModules(url) {
   return StringPrototypeIncludes(url.pathname, '/node_modules/');
 }
 
+/**
+ * Determine whether the given source contains CJS or ESM module syntax.
+ * @param {string} source
+ * @param {URL} url
+ */
+function detectModuleFormat(source, url) {
+  try {
+    let realSource = source ?? readFileSync(url, 'utf8');
+    if (Buffer.isBuffer(realSource)) {
+      // `containsModuleSyntax` requires source to be passed in as string
+      realSource = realSource.toString();
+    }
+    return containsModuleSyntax(realSource, fileURLToPath(url), url) ? 'module' : 'commonjs';
+  } catch {
+    return 'commonjs';
+  }
+}
+
 let typelessPackageJsonFilesWarnedAbout;
 /**
  * @param {URL} url
@@ -113,9 +133,7 @@ function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreE
         // `source` is undefined when this is called from `defaultResolve`;
         // but this gets called again from `defaultLoad`/`defaultLoadSync`.
         if (getOptionValue('--experimental-detect-module')) {
-          const format = source ?
-            (containsModuleSyntax(`${source}`, fileURLToPath(url), url) ? 'module' : 'commonjs') :
-            null;
+          const format = detectModuleFormat(source, url);
           if (format === 'module') {
             // This module has a .js extension, a package.json with no `type` field, and ESM syntax.
             // Warn about the missing `type` field so that the user can avoid the performance penalty of detection.
@@ -155,12 +173,8 @@ function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreE
       }
       default: { // The user did not pass `--experimental-default-type`.
         if (getOptionValue('--experimental-detect-module')) {
-          if (!source) { return null; }
           const format = getFormatOfExtensionlessFile(url);
-          if (format === 'module') {
-            return containsModuleSyntax(`${source}`, fileURLToPath(url), url) ? 'module' : 'commonjs';
-          }
-          return format;
+          return (format === 'module') ? detectModuleFormat(source, url) : format;
         }
         return 'commonjs';
       }

lib/internal/modules/esm/loader.js

@@ -321,7 +321,7 @@ class ModuleLoader {
    * @returns {ModuleJobBase}
    */
   getModuleJobForRequire(specifier, parentURL, importAttributes) {
-    assert(getOptionValue('--experimental-require-module'));
+    assert(getOptionValue('--experimental-require-module') || getOptionValue('--experimental-detect-module'));
 
     if (canParse(specifier)) {
       const protocol = new URL(specifier).protocol;

test/es-module/test-esm-detect-ambiguous.mjs

@@ -155,7 +155,7 @@ describe('--experimental-detect-module', { concurrency: !process.env.TEST_PARALL
       });
     }
 
-    it('should not hint wrong format in resolve hook', async () => {
+    it('should hint format correctly for extensionles modules resolve hook', async () => {
       let writeSync;
       const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
         '--experimental-detect-module',
@@ -172,7 +172,7 @@ describe('--experimental-detect-module', { concurrency: !process.env.TEST_PARALL
       ]);
 
       strictEqual(stderr, '');
-      strictEqual(stdout, 'null\nexecuted\n');
+      strictEqual(stdout, 'module\nexecuted\n');
       strictEqual(code, 0);
       strictEqual(signal, null);
 

test/es-module/test-esm-loader-hooks.mjs

@@ -744,32 +744,41 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => {
     assert.strictEqual(signal, null);
   });
 
-  it('should use ESM loader to respond to require.resolve calls when opting in', async () => {
-    const readFile = async () => {};
-    const fileURLToPath = () => {};
-    const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
-      '--no-warnings',
-      '--experimental-loader',
-      `data:text/javascript,import{readFile}from"node:fs/promises";import{fileURLToPath}from"node:url";export ${
-        async function load(u, c, n) {
-          const r = await n(u, c);
-          if (u.endsWith('/common/index.js')) {
-            r.source = '"use strict";module.exports=require("node:module").createRequire(' +
-                     `${JSON.stringify(u)})(${JSON.stringify(fileURLToPath(u))});\n`;
-          } else if (c.format === 'commonjs') {
-            r.source = await readFile(new URL(u));
-          }
-          return r;
-        }}`,
-      '--experimental-loader',
-      fixtures.fileURL('es-module-loaders/loader-resolve-passthru.mjs'),
-      fixtures.path('require-resolve.js'),
-    ]);
 
-    assert.strictEqual(stderr, '');
-    assert.strictEqual(stdout, 'resolve passthru\n'.repeat(10));
-    assert.strictEqual(code, 0);
-    assert.strictEqual(signal, null);
+  describe('should use ESM loader to respond to require.resolve calls when opting in', () => {
+    for (const { testConfigName, additionalOptions } of [
+      { testConfigName: 'without --experimental-detect-module', additionalOptions: [] },
+      { testConfigName: 'with --experimental-detect-module', additionalOptions: ['--experimental-detect-module'] },
+    ]) {
+      it(testConfigName, async () => {
+        const readFile = async () => {};
+        const fileURLToPath = () => {};
+        const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
+          ...additionalOptions,
+          '--no-warnings',
+          '--experimental-loader',
+          `data:text/javascript,import{readFile}from"node:fs/promises";import{fileURLToPath}from"node:url";export ${
+            async function load(u, c, n) {
+              const r = await n(u, c);
+              if (u.endsWith('/common/index.js')) {
+                r.source = '"use strict";module.exports=require("node:module").createRequire(' +
+                         `${JSON.stringify(u)})(${JSON.stringify(fileURLToPath(u))});\n`;
+              } else if (c.format === 'commonjs') {
+                r.source = await readFile(new URL(u));
+              }
+              return r;
+            }}`,
+          '--experimental-loader',
+          fixtures.fileURL('es-module-loaders/loader-resolve-passthru.mjs'),
+          fixtures.path('require-resolve.js'),
+        ]);
+
+        assert.strictEqual(stderr, '');
+        assert.strictEqual(stdout, 'resolve passthru\n'.repeat(10));
+        assert.strictEqual(code, 0);
+        assert.strictEqual(signal, null);
+      });
+    }
   });
 
   it('should support source maps in commonjs translator', async () => {

test/es-module/test-esm-named-exports-detect-module.js

@@ -0,0 +1,13 @@
+// Flags: --experimental-detect-module --import ./test/fixtures/es-module-loaders/builtin-named-exports.mjs
+'use strict';
+
+const common = require('../common');
+common.skipIfWorker();
+
+const { readFile, __fromLoader } = require('fs');
+const assert = require('assert');
+
+assert.throws(() => require('../fixtures/es-modules/test-esm-ok.mjs'), { code: 'ERR_REQUIRE_ESM' });
+
+assert(readFile);
+assert(__fromLoader);