module: resolve format for all situations with module detection on

Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>
nodejs · Jun 25, 2024 · 5753afc · 5753afc
1 parent c681f57
commit 5753afc
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 53 deletions.
diff --git a/lib/internal/modules/esm/get_format.js b/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';
       }

diff --git a/lib/internal/modules/esm/loader.js b/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) {

diff --git a/src/node_contextify.cc b/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) {

diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/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`

diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs
@@ -744,45 +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 hooks', async () => {
-    const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
-      '--import',
-      fixtures.fileURL('es-module-loaders/builtin-named-exports.mjs'),
-      fixtures.path('es-modules/require-esm-throws-with-loaders.js'),
-    ]);
+  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, '');
-    assert.strictEqual(code, 0);
-    assert.strictEqual(signal, null);
+        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 () => {

diff --git a/test/fixtures/es-module-loaders/transpile-esm-to-cjs.mjs b/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;
+}
diff --git a/test/fixtures/es-modules/package-without-type/imports-nonexistent.js b/test/fixtures/es-modules/package-without-type/imports-nonexistent.js
@@ -0,0 +1 @@
+import "./does-not-exist.js";