review comments, rework tests

nodejs · Dec 17, 2021 · 435dc9b · 435dc9b
1 parent 6f7d871
commit 435dc9b
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 75 deletions.
diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js
@@ -32,6 +32,8 @@ const legacyExtensionFormatMap = {
   '.node': 'commonjs'
 };
 
+let experimentalSpecifierResolutionWarned = false;
+
 if (experimentalWasmModules)
   extensionFormatMap['.wasm'] = legacyExtensionFormatMap['.wasm'] = 'wasm';
 
@@ -64,9 +66,12 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), {
     }
     if (!format) {
       if (experimentalSpecifierResolution === 'node') {
-        process.emitWarning(
-          'The Node.js specifier resolution in ESM is experimental.',
-          'ExperimentalWarning');
+        if (!experimentalSpecifierResolutionWarned) {
+          process.emitWarning(
+            'The Node.js specifier resolution in ESM is experimental.',
+            'ExperimentalWarning');
+          experimentalSpecifierResolutionWarned = true;
+        }
         format = legacyExtensionFormatMap[ext];
       } else {
         throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url));
@@ -90,4 +95,5 @@ module.exports = {
   defaultGetFormat,
   extensionFormatMap,
   legacyExtensionFormatMap,
+  experimentalSpecifierResolutionWarned
 };
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -469,30 +469,34 @@ function resolvePackageTargetString(
 
   const composeResult = (resolved) => {
     let format;
-    try {
-      const ext = extname(resolved.pathname);
-      if (ext === '.js') {
+
+    const ext = extname(resolved.pathname);
+    if (ext === '.js') {
+      try {
         format = getPackageType(resolved);
-      } else {
-        format = extensionFormatMap[ext];
-      }
-      if (!format) {
-        if (experimentalSpecifierResolution === 'node') {
-          process.emitWarning(
-            'The Node.js specifier resolution in ESM is experimental.',
-            'ExperimentalWarning');
-          format = legacyExtensionFormatMap[ext];
+      } catch (err) {
+        if (err.code === 'ERR_INVALID_FILE_URL_PATH') {
+          const invalidModuleErr = new ERR_INVALID_MODULE_SPECIFIER(
+            resolved, 'must not include encoded "/" or "\\" characters', base);
+          invalidModuleErr.cause = err;
+          throw invalidModuleErr;
         }
+        throw err;
       }
-    } catch (err) {
-      if (err.code === 'ERR_INVALID_FILE_URL_PATH') {
-        const invalidModuleErr = new ERR_INVALID_MODULE_SPECIFIER(
-          resolved, 'must not include encoded "/" or "\\" characters', base);
-        invalidModuleErr.cause = err;
-        throw invalidModuleErr;
+    } else {
+      format = extensionFormatMap[ext];
+    }
+
+    if (format == null && experimentalSpecifierResolution === 'node') {
+      if (!experimentalSpecifierResolutionWarned) {
+        process.emitWarning(
+          'The Node.js specifier resolution in ESM is experimental.',
+          'ExperimentalWarning');
+        experimentalSpecifierResolutionWarned = true;
       }
-      throw err;
+      format = legacyExtensionFormatMap[ext];
     }
+
     return { resolved, ...(format !== 'none') && { format } };
   };
 
@@ -1123,6 +1127,9 @@ module.exports = {
 };
 
 // cycle
-const { defaultGetFormat,
-        extensionFormatMap,
-        legacyExtensionFormatMap } = require('internal/modules/esm/get_format');
+let {
+  defaultGetFormat,
+  extensionFormatMap,
+  legacyExtensionFormatMap,
+  experimentalSpecifierResolutionWarned,
+} = require('internal/modules/esm/get_format');
diff --git a/test/es-module/test-esm-resolve-type.js b/test/es-module/test-esm-resolve-type.js
@@ -181,69 +181,49 @@ try {
     assert.ok(resolveResult.url.includes('my-dual-package/es/index.js'));
   }
 
-  function testDualPackageWithMjsMainScriptAndCJSType() {
+  testDualPackageWithJsMainScriptAndModuleType();
 
-    // Additional test for following scenario
-    /**
-     * this creates following directory structure:
-     *
-     * ./node_modules:
-     *   |-> dual-mjs-pjson
-     *       |-> subdir
-     *           |-> index.mjs [3]
-     *           |-> package.json [2]
-     *           |-> index.js
-     *       |->package.json [1]
-     *
-     * [1] - main package.json of the package
-     *     - it contains:
-     *             - type: 'commonjs'
-     *             - main: 'subdir/index.js'
-     *             - conditional exports for 'require' (subdir/index.js) and
-     *                                       'import' (subdir/index.mjs)
-     * [2] - package.json add-on for the import case
-     *     - it only contains:
-     *             - type: 'commonjs'
-     * [3] - main script for the `import` case
-     *
-     * in case the package is consumed as an ESM by importing it:
-     *    import * as my-package from 'dual-mjs-pjson'
-     * it will cause the resolve method to return:
-     *  {
-     *     url: '<base_path>/node_modules/dual-mjs-pjson/subdir/index.mjs',
-     *     format: 'module'
-     *  }
-     *
-     *  following testcase ensures that resolve works correctly in this case
-     *  returning the information as specified above. Source for 'url' value
-     *  is [1], source for 'format' value is the file extension of [3]
-     */
-    const moduleName = 'dual-mjs-pjson';
+  // TestParameters are ModuleName, mainRequireScript, mainImportScript,
+  // mainPackageType, subdirPkgJsonType, expectedResolvedFormat
+  [ [ 'mjs-mod-mod', 'index.js', 'index.mjs', 'module', 'module', 'module'],
+    [ 'mjs-com-com', 'idx.js', 'idx.mjs', 'commonjs', 'commonjs', 'module'],
+    [ 'mjs-mod-com', 'index.js', 'imp.mjs', 'module', 'commonjs', 'module'],
+    [ 'js-com-com', 'index.js', 'imp.js', 'commonjs', 'commonjs', 'commonjs'],
+    [ 'js-com-mod', 'index.js', 'imp.js', 'commonjs', 'module', 'module'],
+    [ 'ts-mod-com', 'index.js', 'imp.ts', 'module', 'commonjs', undefined],
+  ].forEach((testVariant) => {
+    const [
+      moduleName,
+      mainRequireScript,
+      mainImportScript,
+      mainPackageType,
+      subdirPackageType,
+      expectedResolvedFormat ] = testVariant;
 
     const mDir = rel(`node_modules/${moduleName}`);
     const subDir = rel(`node_modules/${moduleName}/subdir`);
     const pkg = rel(`node_modules/${moduleName}/package.json`);
     const subdirPkg = rel(`node_modules/${moduleName}/subdir/package.json`);
-    const esScript = rel(`node_modules/${moduleName}/subdir/index.mjs`);
-    const cjsScript = rel(`node_modules/${moduleName}/subdir/index.js`);
+    const esScript = rel(`node_modules/${moduleName}/subdir/${mainImportScript}`);
+    const cjsScript = rel(`node_modules/${moduleName}/subdir/${mainRequireScript}`);
 
     createDir(nmDir);
     createDir(mDir);
     createDir(subDir);
 
     const mainPkgJsonContent = {
-      type: 'commonjs',
-      main: 'lib/index.js',
+      type: mainPackageType,
+      main: `./subdir/${mainRequireScript}`,
       exports: {
         '.': {
-          'require': './subdir/index.js',
-          'import': './subdir/index.mjs'
+          'require': `./subdir/${mainRequireScript}`,
+          'import': `./subdir/${mainImportScript}`
         },
         './package.json': './package.json',
       }
     };
     const subdirPkgJsonContent = {
-      type: 'commonjs'
+      type: `${subdirPackageType}`
     };
 
     fs.writeFileSync(pkg, JSON.stringify(mainPkgJsonContent));
@@ -257,12 +237,9 @@ try {
 
     // test the resolve
     const resolveResult = resolve(`${moduleName}`);
-    assert.strictEqual(resolveResult.format, 'module');
-    assert.ok(resolveResult.url.includes(`${moduleName}/subdir/index.mjs`));
-  }
-
-  testDualPackageWithJsMainScriptAndModuleType();
-  testDualPackageWithMjsMainScriptAndCJSType();
+    assert.strictEqual(resolveResult.format, expectedResolvedFormat);
+    assert.ok(resolveResult.url.includes(`${moduleName}/subdir/${mainImportScript}`));
+  });
 
 } finally {
   process.chdir(previousCwd);