loader: fix package resolution for edge case

dygabo · Flarna · commit ffa49d28ff75 · 2022-01-31T17:27:58.000+01:00
this commit solves a regression introduced with PR-40980. if a resolve call results in a script with .mjs extension the is automatically set to . This avoids the case where an additional in the same directory as the .mjs file would declare the to commonjs PR-URL: nodejs#41218 Refs: nodejs#40980 Refs: yargs/yargs#2068 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: nodejs#41752
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';
 
@@ -53,41 +55,57 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), {
 
     return format;
   },
-  'file:'(parsed, url) {
-    const ext = extname(parsed.pathname);
-    let format;
-
-    if (ext === '.js') {
-      format = getPackageType(parsed.href) === 'module' ? 'module' : 'commonjs';
-    } else {
-      format = extensionFormatMap[ext];
-    }
-    if (!format) {
-      if (experimentalSpecifierResolution === 'node') {
-        process.emitWarning(
-          'The Node.js specifier resolution in ESM is experimental.',
-          'ExperimentalWarning');
-        format = legacyExtensionFormatMap[ext];
-      } else {
-        throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url));
-      }
-    }
-
-    return format || null;
-  },
+  'file:': getFileProtocolModuleFormat,
   'node:'() { return 'builtin'; },
 });
 
-function defaultGetFormat(url, context) {
+function getLegacyExtensionFormat(ext) {
+  if (
+    experimentalSpecifierResolution === 'node' &&
+    !experimentalSpecifierResolutionWarned
+  ) {
+    process.emitWarning(
+      'The Node.js specifier resolution in ESM is experimental.',
+      'ExperimentalWarning');
+    experimentalSpecifierResolutionWarned = true;
+  }
+  return legacyExtensionFormatMap[ext];
+}
+
+function getFileProtocolModuleFormat(url, ignoreErrors) {
+  const ext = extname(url.pathname);
+  if (ext === '.js') {
+    return getPackageType(url) === 'module' ? 'module' : 'commonjs';
+  }
+
+  const format = extensionFormatMap[ext];
+  if (format) return format;
+  if (experimentalSpecifierResolution !== 'node') {
+    // Explicit undefined return indicates load hook should rerun format check
+    if (ignoreErrors)
+      return undefined;
+    throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url));
+  }
+  return getLegacyExtensionFormat(ext) ?? null;
+}
+
+function defaultGetFormatWithoutErrors(url, context) {
   const parsed = new URL(url);
+  if (!ObjectPrototypeHasOwnProperty(protocolHandlers, parsed.protocol))
+    return null;
+  return protocolHandlers[parsed.protocol](parsed, true);
+}
 
+function defaultGetFormat(url, context) {
+  const parsed = new URL(url);
   return ObjectPrototypeHasOwnProperty(protocolHandlers, parsed.protocol) ?
-    protocolHandlers[parsed.protocol](parsed, url) :
+    protocolHandlers[parsed.protocol](parsed, false) :
     null;
 }
 
 module.exports = {
   defaultGetFormat,
+  defaultGetFormatWithoutErrors,
   extensionFormatMap,
   legacyExtensionFormatMap,
 };
diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js
@@ -2,15 +2,16 @@
 
 const { defaultGetFormat } = require('internal/modules/esm/get_format');
 const { defaultGetSource } = require('internal/modules/esm/get_source');
-const { translators } = require('internal/modules/esm/translators');
 
 async function defaultLoad(url, context) {
   let {
     format,
     source,
   } = context;
 
-  if (!translators.has(format)) format = defaultGetFormat(url);
+  if (format == null) {
+    format = defaultGetFormat(url);
+  }
 
   if (
     format === 'builtin' ||
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -131,7 +131,7 @@ function emitTrailingSlashPatternDeprecation(match, pjsonUrl, isExports, base) {
  * @returns
  */
 function emitLegacyIndexDeprecation(url, packageJSONUrl, base, main) {
-  const format = defaultGetFormat(url);
+  const format = defaultGetFormatWithoutErrors(url);
   if (format !== 'module')
     return;
   const path = fileURLToPath(url);
@@ -488,22 +488,6 @@ const patternRegEx = /\*/g;
 function resolvePackageTargetString(
   target, subpath, match, packageJSONUrl, base, pattern, internal, conditions) {
 
-  const composeResult = (resolved) => {
-    let format;
-    try {
-      format = getPackageType(resolved);
-    } 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;
-    }
-    return { resolved, ...(format !== 'none') && { format } };
-  };
-
   if (subpath !== '' && !pattern && target[target.length - 1] !== '/')
     throwInvalidPackageTarget(match, target, packageJSONUrl, internal, base);
 
@@ -536,18 +520,22 @@ function resolvePackageTargetString(
   if (!StringPrototypeStartsWith(resolvedPath, packagePath))
     throwInvalidPackageTarget(match, target, packageJSONUrl, internal, base);
 
-  if (subpath === '') return composeResult(resolved);
+  if (subpath === '') return resolved;
 
   if (RegExpPrototypeTest(invalidSegmentRegEx, subpath))
     throwInvalidSubpath(match + subpath, packageJSONUrl, internal, base);
 
   if (pattern) {
-    return composeResult(new URL(RegExpPrototypeSymbolReplace(patternRegEx,
-                                                              resolved.href,
-                                                              () => subpath)));
+    return new URL(
+      RegExpPrototypeSymbolReplace(
+        patternRegEx,
+        resolved.href,
+        () => subpath
+      )
+    );
   }
 
-  return composeResult(new URL(subpath, resolved));
+  return new URL(subpath, resolved);
 }
 
 /**
@@ -673,15 +661,15 @@ function packageExportsResolve(
       !StringPrototypeIncludes(packageSubpath, '*') &&
       !StringPrototypeEndsWith(packageSubpath, '/')) {
     const target = exports[packageSubpath];
-    const resolveResult = resolvePackageTarget(
+    const resolved = resolvePackageTarget(
       packageJSONUrl, target, '', packageSubpath, base, false, false, conditions
     );
 
-    if (resolveResult == null) {
+    if (resolved == null) {
       throwExportsNotFound(packageSubpath, packageJSONUrl, base);
     }
 
-    return { ...resolveResult, exact: true };
+    return { resolved, exact: true };
   }
 
   let bestMatch = '';
@@ -717,7 +705,7 @@ function packageExportsResolve(
   if (bestMatch) {
     const target = exports[bestMatch];
     const pattern = StringPrototypeIncludes(bestMatch, '*');
-    const resolveResult = resolvePackageTarget(
+    const resolved = resolvePackageTarget(
       packageJSONUrl,
       target,
       bestMatchSubpath,
@@ -727,15 +715,15 @@ function packageExportsResolve(
       false,
       conditions);
 
-    if (resolveResult == null) {
+    if (resolved == null) {
       throwExportsNotFound(packageSubpath, packageJSONUrl, base);
     }
 
     if (!pattern) {
       emitFolderMapDeprecation(bestMatch, packageJSONUrl, true, base);
     }
 
-    return { ...resolveResult, exact: pattern };
+    return { resolved, exact: pattern };
   }
 
   throwExportsNotFound(packageSubpath, packageJSONUrl, base);
@@ -775,11 +763,11 @@ function packageImportsResolve(name, base, conditions) {
       if (ObjectPrototypeHasOwnProperty(imports, name) &&
           !StringPrototypeIncludes(name, '*') &&
           !StringPrototypeEndsWith(name, '/')) {
-        const resolveResult = resolvePackageTarget(
+        const resolved = resolvePackageTarget(
           packageJSONUrl, imports[name], '', name, base, false, true, conditions
         );
-        if (resolveResult != null) {
-          return { resolved: resolveResult.resolved, exact: true };
+        if (resolved != null) {
+          return { resolved, exact: true };
         }
       } else {
         let bestMatch = '';
@@ -812,15 +800,15 @@ function packageImportsResolve(name, base, conditions) {
         if (bestMatch) {
           const target = imports[bestMatch];
           const pattern = StringPrototypeIncludes(bestMatch, '*');
-          const resolveResult = resolvePackageTarget(
+          const resolved = resolvePackageTarget(
             packageJSONUrl, target,
             bestMatchSubpath, bestMatch,
             base, pattern, true,
             conditions);
-          if (resolveResult !== null) {
+          if (resolved !== null) {
             if (!pattern)
               emitFolderMapDeprecation(bestMatch, packageJSONUrl, false, base);
-            return { resolved: resolveResult.resolved, exact: pattern };
+            return { resolved, exact: pattern };
           }
         }
       }
@@ -880,7 +868,7 @@ function parsePackageName(specifier, base) {
  * @param {string} specifier
  * @param {string | URL | undefined} base
  * @param {Set<string>} conditions
- * @returns {resolved: URL, format? : string}
+ * @returns {URL}
  */
 function packageResolve(specifier, base, conditions) {
   if (NativeModule.canBeRequiredByUsers(specifier))
@@ -896,7 +884,8 @@ function packageResolve(specifier, base, conditions) {
     if (packageConfig.name === packageName &&
         packageConfig.exports !== undefined && packageConfig.exports !== null) {
       return packageExportsResolve(
-        packageJSONUrl, packageSubpath, packageConfig, base, conditions);
+        packageJSONUrl, packageSubpath, packageConfig, base, conditions
+      ).resolved;
     }
   }
 
@@ -920,24 +909,19 @@ function packageResolve(specifier, base, conditions) {
     const packageConfig = getPackageConfig(packageJSONPath, specifier, base);
     if (packageConfig.exports !== undefined && packageConfig.exports !== null) {
       return packageExportsResolve(
-        packageJSONUrl, packageSubpath, packageConfig, base, conditions);
+        packageJSONUrl, packageSubpath, packageConfig, base, conditions
+      ).resolved;
     }
 
     if (packageSubpath === '.') {
-      return {
-        resolved: legacyMainResolve(
-          packageJSONUrl,
-          packageConfig,
-          base),
-        ...(packageConfig.type !== 'none') && { format: packageConfig.type }
-      };
+      return legacyMainResolve(
+        packageJSONUrl,
+        packageConfig,
+        base
+      );
     }
 
-    return {
-      resolved: new URL(packageSubpath, packageJSONUrl),
-      ...(packageConfig.type !== 'none') && { format: packageConfig.type }
-    };
-
+    return new URL(packageSubpath, packageJSONUrl);
     // Cross-platform root check.
   } while (packageJSONPath.length !== lastPath.length);
 
@@ -981,7 +965,6 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) {
   // Order swapped from spec for minor perf gain.
   // Ok since relative URLs cannot parse as URLs.
   let resolved;
-  let format;
   if (shouldBeTreatedAsRelativeOrAbsolutePath(specifier)) {
     resolved = new URL(specifier, base);
   } else if (specifier[0] === '#') {
@@ -990,15 +973,12 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) {
     try {
       resolved = new URL(specifier);
     } catch {
-      ({ resolved, format } = packageResolve(specifier, base, conditions));
+      resolved = packageResolve(specifier, base, conditions);
     }
   }
   if (resolved.protocol !== 'file:')
     return resolved;
-  return {
-    url: finalizeResolution(resolved, base, preserveSymlinks),
-    ...(format != null) && { format }
-  };
+  return finalizeResolution(resolved, base, preserveSymlinks);
 }
 
 /**
@@ -1047,6 +1027,13 @@ function resolveAsCommonJS(specifier, parentURL) {
   }
 }
 
+function throwIfUnsupportedURLProtocol(url) {
+  if (url.protocol !== 'file:' && url.protocol !== 'data:' &&
+      url.protocol !== 'node:') {
+    throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(url);
+  }
+}
+
 function defaultResolve(specifier, context = {}, defaultResolveUnused) {
   let { parentURL, conditions } = context;
   if (parentURL && policy?.manifest) {
@@ -1087,15 +1074,9 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) {
 
   conditions = getConditionsSet(conditions);
   let url;
-  let format;
   try {
-    ({ url, format } =
-      moduleResolve(
-        specifier,
-        parentURL,
-        conditions, 
-        isMain ? preserveSymlinksMain : preserveSymlinks
-      ));
+    url = moduleResolve(specifier, parentURL, conditions,
+                        isMain ? preserveSymlinksMain : preserveSymlinks);
   } catch (error) {
     // Try to give the user a hint of what would have been the
     // resolved CommonJS module
@@ -1119,13 +1100,11 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) {
     throw error;
   }
 
-  if (url.protocol !== 'file:' && url.protocol !== 'data:' &&
-      url.protocol !== 'node:')
-    throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(url);
+  throwIfUnsupportedURLProtocol(url);
 
   return {
     url: `${url}`,
-    ...(format != null) && { format }
+    format: defaultGetFormatWithoutErrors(url),
   };
 }
 
@@ -1140,4 +1119,6 @@ module.exports = {
 };
 
 // cycle
-const { defaultGetFormat } = require('internal/modules/esm/get_format');
+const {
+  defaultGetFormatWithoutErrors,
+} = 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
