review comments, test cases added for query and hash export urls

nodejs · nodejs-github-bot · Dec 22, 2021 · Dec 17, 2021 · Dec 17, 2021 · Dec 17, 2021
commit f9fe067bbd2eceb0661090a99e411d8d5c0ea73b
diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js
@@ -62,27 +62,34 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), {
     if (ext === '.js') {
       format = getPackageType(parsed.href) === 'module' ? 'module' : 'commonjs';
     } else {
-      format = extensionFormatMap[ext];
-    }
-    if (!format) {
-      if (experimentalSpecifierResolution === 'node') {
-        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));
-      }
+      format = extensionFormatMap[ext] ??
+        throwIfNotExperimentalSpecifierResolution(ext, url);
     }
 
     return format || null;
   },
   'node:'() { return 'builtin'; },
 });
 
+function throwIfNotExperimentalSpecifierResolution(ext, url) {
+  if (experimentalSpecifierResolution !== 'node') {
+    throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url));
+  }
+  return getLegacyExtensionFormat(ext);
+}
+
+function getLegacyExtensionFormat(ext) {
+  if (experimentalSpecifierResolution === 'node') {
+    if (!experimentalSpecifierResolutionWarned) {
+      process.emitWarning(
+        'The Node.js specifier resolution in ESM is experimental.',
+        'ExperimentalWarning');
+      experimentalSpecifierResolutionWarned = true;
+    }
+  }
+  return legacyExtensionFormatMap[ext];
+}
+
 function defaultGetFormat(url, context) {
   const parsed = new URL(url);
 
@@ -95,5 +102,5 @@ module.exports = {
   defaultGetFormat,
   extensionFormatMap,
   legacyExtensionFormatMap,
-  experimentalSpecifierResolutionWarned
+  getLegacyExtensionFormat
 };
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -62,9 +62,6 @@ const userConditions = getOptionValue('--conditions');
 const noAddons = getOptionValue('--no-addons');
 const addonConditions = noAddons ? [] : ['node-addons'];
 
-const experimentalSpecifierResolution =
-  getOptionValue('--experimental-specifier-resolution');
-
 const DEFAULT_CONDITIONS = ObjectFreeze([
   'node',
   'import',
@@ -484,17 +481,7 @@ function resolvePackageTargetString(
         throw err;
       }
     } 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;
-      }
-      format = legacyExtensionFormatMap[ext];
+      format = extensionFormatMap[ext] ?? getLegacyExtensionFormat(ext);
     }
 
     return { resolved, ...(format !== 'none') && { format } };
@@ -1127,9 +1114,8 @@ module.exports = {
 };
 
 // cycle
-let {
+const {
   defaultGetFormat,
   extensionFormatMap,
-  legacyExtensionFormatMap,
-  experimentalSpecifierResolutionWarned,
+  getLegacyExtensionFormat,
 } = 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
@@ -184,12 +184,15 @@ try {
   testDualPackageWithJsMainScriptAndModuleType();
 
   // TestParameters are ModuleName, mainRequireScript, mainImportScript,
-  // mainPackageType, subdirPkgJsonType, expectedResolvedFormat
+  // mainPackageType, subdirPkgJsonType, expectedResolvedFormat, mainSuffix
   [ [ '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'],
+    [ 'qmod', 'index.js', 'imp.js', 'commonjs', 'module', 'module', '?k=v'],
+    [ 'hmod', 'index.js', 'imp.js', 'commonjs', 'module', 'module', '#Key'],
+    [ 'qhmod', 'index.js', 'imp.js', 'commonjs', 'module', 'module', '?k=v#h'],
     [ 'ts-mod-com', 'index.js', 'imp.ts', 'module', 'commonjs', undefined],
   ].forEach((testVariant) => {
     const [
@@ -198,7 +201,8 @@ try {
       mainImportScript,
       mainPackageType,
       subdirPackageType,
-      expectedResolvedFormat ] = testVariant;
+      expectedResolvedFormat,
+      mainSuffix ] = testVariant;
 
     const mDir = rel(`node_modules/${moduleName}`);
     const subDir = rel(`node_modules/${moduleName}/subdir`);
@@ -211,13 +215,14 @@ try {
     createDir(mDir);
     createDir(subDir);
 
+    const mainScript = mainImportScript + (mainSuffix ?? '');
     const mainPkgJsonContent = {
       type: mainPackageType,
       main: `./subdir/${mainRequireScript}`,
       exports: {
         '.': {
           'require': `./subdir/${mainRequireScript}`,
-          'import': `./subdir/${mainImportScript}`
+          'import': `./subdir/${mainScript}`
         },
         './package.json': './package.json',
       }