esm: fix format sometimes being null

nodejs · Jul 15, 2024 · aadf162 · aadf162
1 parent 86415e4
commit aadf162
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 28 deletions.
diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js
@@ -17,6 +17,7 @@ const {
   mimeToFormat,
 } = require('internal/modules/esm/formats');
 
+const detectModule = getOptionValue('--experimental-detect-module');
 const experimentalNetworkImports =
   getOptionValue('--experimental-network-imports');
 const { containsModuleSyntax } = internalBinding('contextify');
@@ -33,6 +34,17 @@ const protocolHandlers = {
   'node:'() { return 'builtin'; },
 };
 
+/**
+ * Determine whether the given ambiguous source contains CommonJS or ES module syntax.
+ * @param {string | Buffer | undefined} source
+ * @param {URL} url
+ */
+function detectModuleFormat(source, url) {
+  if (!source) { return detectModule ? undefined : 'commonjs'; }
+  if (!detectModule) { return 'commonjs'; }
+  return containsModuleSyntax(`${source}`, fileURLToPath(url), url) ? 'module' : 'commonjs';
+}
+
 /**
  * @param {URL} parsed
  * @returns {string | null}
@@ -112,26 +124,23 @@ function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreE
       default: { // The user did not pass `--experimental-default-type`.
         // `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;
-          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.
-            typelessPackageJsonFilesWarnedAbout ??= new SafeSet();
-            if (!typelessPackageJsonFilesWarnedAbout.has(pjsonPath)) {
-              const warning = `${url} parsed as an ES module because module syntax was detected;` +
-              ` to avoid the performance penalty of syntax detection, add "type": "module" to ${pjsonPath}`;
-              process.emitWarning(warning, {
-                code: 'MODULE_TYPELESS_PACKAGE_JSON',
-              });
-              typelessPackageJsonFilesWarnedAbout.add(pjsonPath);
-            }
+        // For ambiguous files (no type field, .js extension) we return
+        // undefined from `resolve` and re-run the check in `load`.
+        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.
+          typelessPackageJsonFilesWarnedAbout ??= new SafeSet();
+          if (!typelessPackageJsonFilesWarnedAbout.has(pjsonPath)) {
+            const warning = `${url} parsed as an ES module because module syntax was detected;` +
+            ` to avoid the performance penalty of syntax detection, add "type": "module" to ${pjsonPath}`;
+            process.emitWarning(warning, {
+              code: 'MODULE_TYPELESS_PACKAGE_JSON',
+            });
+            typelessPackageJsonFilesWarnedAbout.add(pjsonPath);
           }
-          return format;
         }
-        return 'commonjs';
+        return format;
       }
     }
   }
@@ -154,15 +163,14 @@ function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreE
         return 'commonjs';
       }
       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';
-          }
+        if (!source) {
+          return;
+        }
+        const format = getFormatOfExtensionlessFile(url);
+        if (format === 'wasm') {
           return format;
         }
-        return 'commonjs';
+        return detectModuleFormat(source, url);
       }
     }
   }

diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs
@@ -175,8 +175,8 @@ describe('--experimental-detect-module', { concurrency: !process.env.TEST_PARALL
         '--no-warnings',
         '--loader',
         `data:text/javascript,import { writeSync } from "node:fs"; export ${encodeURIComponent(
-          async function resolve(s, c, next) {
-            const result = await next(s, c);
+          async function resolve(specifier, context, next) {
+            const result = await next(specifier, context);
             writeSync(1, result.format + '\n');
             return result;
           }
@@ -185,7 +185,7 @@ describe('--experimental-detect-module', { concurrency: !process.env.TEST_PARALL
       ]);
 
       strictEqual(stderr, '');
-      strictEqual(stdout, 'null\nexecuted\n');
+      strictEqual(stdout, 'undefined\nexecuted\n');
       strictEqual(code, 0);
       strictEqual(signal, null);