module: resolve format for all situations with module detection on

Co-authored-by: Geoffrey Booth <[email protected]>

Author: dygabo

lib/internal/modules/esm/get_format.js

@@ -82,6 +82,17 @@ function underNodeModules(url) {
   return StringPrototypeIncludes(url.pathname, '/node_modules/');
 }
 
+/**
+ * Determine whether the given source contains CJS or ESM module syntax.
+ * @param {string | Buffer | undefined} source
+ * @param {URL} url
+ */
+function detectModuleFormat(source, url) {
+  const moduleSource = source ? `${source}` : undefined;
+  const modulePath = fileURLToPath(url);
+  return containsModuleSyntax(moduleSource, modulePath, url) ? 'module' : 'commonjs';
+}
+
 let typelessPackageJsonFilesWarnedAbout;
 /**
  * @param {URL} url
@@ -113,9 +124,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 +164,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

@@ -329,7 +329,7 @@ class ModuleLoader {
    * @returns {ModuleJobBase}
    */
   getModuleJobForRequire(specifier, parentURL, importAttributes) {
-    assert(getOptionValue('--experimental-require-module'));
+    assert(getOptionValue('--experimental-require-module') || getOptionValue('--experimental-detect-module'));
 
     const parsed = URLParse(specifier);
     if (parsed != null) {

src/node_contextify.cc

@@ -1705,14 +1705,32 @@ static void ContainsModuleSyntax(const FunctionCallbackInfo<Value>& args) {
 
   CHECK_GE(args.Length(), 2);
 
-  // Argument 1: source code
-  CHECK(args[0]->IsString());
-  Local<String> code = args[0].As<String>();
-
   // Argument 2: filename
   CHECK(args[1]->IsString());
   Local<String> filename = args[1].As<String>();
 
+  // Argument 1: source code; if undefined, read from filename in argument 2
+  Local<String> code;
+  if (args[0]->IsUndefined()) {
+    CHECK(!filename.IsEmpty());
+    const char* filename_str = Utf8Value(isolate, filename).out();
+    std::string contents;
+    int result = ReadFileSync(&contents, filename_str);
+    if (result != 0) {
+      isolate->ThrowException(
+          ERR_MODULE_NOT_FOUND(isolate, "Cannot read file %s", filename_str));
+      return;
+    }
+    code = String::NewFromUtf8(isolate,
+                               contents.c_str(),
+                               v8::NewStringType::kNormal,
+                               contents.length())
+               .ToLocalChecked();
+  } else {
+    CHECK(args[0]->IsString());
+    code = args[0].As<String>();
+  }
+
   // Argument 3: resource name (URL for ES module).
   Local<String> resource_name = filename;
   if (args[2]->IsString()) {
@@ -1729,6 +1747,7 @@ static void ContainsModuleSyntax(const FunctionCallbackInfo<Value>& args) {
     Local<Function> fn;
     TryCatchScope try_catch(env);
     ShouldNotAbortOnUncaughtScope no_abort_scope(env);
+
     if (CompileFunctionForCJSLoader(
             env, context, code, filename, &cache_rejected, cjs_var)
             .ToLocal(&fn)) {
@@ -1740,7 +1759,13 @@ static void ContainsModuleSyntax(const FunctionCallbackInfo<Value>& args) {
   }
 
   bool result = ShouldRetryAsESM(realm, message, code, resource_name);
-  args.GetReturnValue().Set(result);
+  if (result) {
+    // successfully parsed as ESM after failing to parse as CJS => ESM syntax
+    args.GetReturnValue().Set(result);
+    return;
+  }
+
+  args.GetReturnValue().SetUndefined();
 }
 
 static void StartSigintWatchdog(const FunctionCallbackInfo<Value>& args) {

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

@@ -168,7 +168,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 the resolve hook for extensionless modules', async () => {
       let writeSync;
       const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
         '--experimental-detect-module',
@@ -185,7 +185,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);
 
@@ -385,6 +385,65 @@ describe('--experimental-detect-module', { concurrency: !process.env.TEST_PARALL
       strictEqual(signal, null);
     });
   });
+
+  describe('should work with module customization hooks', { concurrency: true }, () => {
+    it('should not break basic hooks functionality of substituting a module', async () => {
+      const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+        '--experimental-detect-module',
+        '--import',
+        fixtures.fileURL('es-module-loaders/builtin-named-exports.mjs'),
+        fixtures.path('es-modules/require-esm-throws-with-loaders.js'),
+      ]);
+
+      strictEqual(stderr, '');
+      strictEqual(stdout, '');
+      strictEqual(code, 0);
+      strictEqual(signal, null);
+    });
+
+    it('should detect the syntax of the source as returned by a custom load hook', async () => {
+      const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+        '--no-warnings',
+        '--experimental-detect-module',
+        '--import',
+        `data:text/javascript,${encodeURIComponent(
+          'import { register } from "node:module";' +
+          'import { pathToFileURL } from "node:url";' +
+          'register("./transpile-esm-to-cjs.mjs", pathToFileURL("./"));'
+        )}`,
+        fixtures.path('es-modules/package-without-type/module.js'),
+      ], { cwd: fixtures.fileURL('es-module-loaders/') });
+
+      strictEqual(stderr, '');
+      strictEqual(stdout, `
+        Resolved format: module
+        Loaded original format: module
+        executed
+        Evaluated format: commonjs
+        `.replace(/^\s+/gm, ''));
+      strictEqual(code, 0);
+      strictEqual(signal, null);
+    });
+
+    it('should throw the usual error for a missing file', async () => {
+      const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
+        '--no-warnings',
+        '--experimental-detect-module',
+        '--import',
+        `data:text/javascript,${encodeURIComponent(
+          'import { register } from "node:module";' +
+          'import { pathToFileURL } from "node:url";' +
+          'register("./transpile-esm-to-cjs.mjs", pathToFileURL("./"));'
+        )}`,
+        fixtures.path('es-modules/package-without-type/imports-nonexistent.js'),
+      ], { cwd: fixtures.fileURL('es-module-loaders/') });
+
+      match(stderr, /ERR_MODULE_NOT_FOUND.+does-not-exist\.js/);
+      strictEqual(stdout, 'Resolved format: module\nLoaded original format: module\n');
+      strictEqual(code, 1);
+      strictEqual(signal, null);
+    });
+  });
 });
 
 // Validate temporarily disabling `--abort-on-uncaught-exception`

test/fixtures/es-module-loaders/transpile-esm-to-cjs.mjs

@@ -0,0 +1,28 @@
+import { writeSync } from "node:fs";
+
+export async function resolve(specifier, context, next) {
+  const result = await next(specifier, context);
+  if (specifier.startsWith("file://")) {
+    writeSync(1, `Resolved format: ${result.format}\n`);
+  }
+  return result;
+}
+
+export async function load(url, context, next) {
+  const output = await next(url, context);
+  writeSync(1, `Loaded original format: ${output.format}\n`);
+
+  let source = `${output.source}`
+
+  // This is a very incomplete and naively done implementation for testing purposes only
+  if (source?.includes('export default')) {
+    source = source.replace('export default', 'module.exports =');
+
+    source += '\nconsole.log(`Evaluated format: ${this === undefined ? "module" : "commonjs"}`);';
+
+    output.source = source;
+    output.format = 'commonjs';
+  }
+
+  return output;
+}

test/fixtures/es-modules/package-without-type/imports-nonexistent.js

@@ -0,0 +1 @@
+import "./does-not-exist.js";