lib: merge cjs and esm package json reader caches

nodejs · Jun 17, 2023 · 7806a59 · 7806a59
1 parent 0d725d6
commit 7806a59
Show file tree

Hide file tree

Showing 7 changed files with 119 additions and 216 deletions.
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -82,7 +82,6 @@ const {
   pendingDeprecate,
   emitExperimentalWarning,
   kEmptyObject,
-  filterOwnProperties,
   setOwnProperty,
   getLazy,
 } = require('internal/util');
@@ -353,36 +352,12 @@ function initializeCJS() {
 //   -> a.<ext>
 //   -> a/index.<ext>
 
-const packageJsonCache = new SafeMap();
-
+/**
+ * @param {string} requestPath
+ * @return {PackageConfig}
+ */
 function readPackage(requestPath) {
-  const jsonPath = path.resolve(requestPath, 'package.json');
-
-  const existing = packageJsonCache.get(jsonPath);
-  if (existing !== undefined) return existing;
-
-  const result = packageJsonReader.read(jsonPath);
-  const json = result.containsKeys === false ? '{}' : result.string;
-  if (json === undefined) {
-    packageJsonCache.set(jsonPath, false);
-    return false;
-  }
-
-  try {
-    const filtered = filterOwnProperties(JSONParse(json), [
-      'name',
-      'main',
-      'exports',
-      'imports',
-      'type',
-    ]);
-    packageJsonCache.set(jsonPath, filtered);
-    return filtered;
-  } catch (e) {
-    e.path = jsonPath;
-    e.message = 'Error parsing ' + jsonPath + ': ' + e.message;
-    throw e;
-  }
+  return packageJsonReader.read(path.resolve(requestPath, 'package.json'));
 }
 
 let _readPackage = readPackage;
@@ -412,7 +387,7 @@ function readPackageScope(checkPath) {
     if (StringPrototypeEndsWith(checkPath, sep + 'node_modules'))
       return false;
     const pjson = _readPackage(checkPath + sep);
-    if (pjson) return {
+    if (pjson.exists) return {
       data: pjson,
       path: checkPath,
     };
@@ -421,7 +396,7 @@ function readPackageScope(checkPath) {
 }
 
 function tryPackage(requestPath, exts, isMain, originalPath) {
-  const pkg = _readPackage(requestPath)?.main;
+  const pkg = _readPackage(requestPath).main;
 
   if (!pkg) {
     return tryExtensions(path.resolve(requestPath, 'index'), exts, isMain);
@@ -526,9 +501,10 @@ function trySelfParentPath(parent) {
 function trySelf(parentPath, request) {
   if (!parentPath) return false;
 
-  const { data: pkg, path: pkgPath } = readPackageScope(parentPath) || {};
-  if (!pkg || pkg.exports === undefined) return false;
-  if (typeof pkg.name !== 'string') return false;
+  const { data: pkg, path: pkgPath } = readPackageScope(parentPath);
+  if (!pkg || pkg.exports == null || pkg.name === undefined) {
+    return false;
+  }
 
   let expansion;
   if (request === pkg.name) {
@@ -563,7 +539,7 @@ function resolveExports(nmPath, request) {
     return;
   const pkgPath = path.resolve(nmPath, name);
   const pkg = _readPackage(pkgPath);
-  if (pkg?.exports != null) {
+  if (pkg.exists && pkg.exports != null) {
     try {
       const { packageExportsResolve } = require('internal/modules/esm/resolve');
       return finalizeEsmResolution(packageExportsResolve(
@@ -1272,9 +1248,9 @@ Module._extensions['.js'] = function(module, filename) {
     content = fs.readFileSync(filename, 'utf8');
   }
   if (StringPrototypeEndsWith(filename, '.js')) {
-    const pkg = readPackageScope(filename);
+    const pkg = readPackageScope(filename) || {};
     // Function require shouldn't be used in ES modules.
-    if (pkg?.data?.type === 'module') {
+    if (pkg.data?.type === 'module') {
       const parent = moduleParentCache.get(module);
       const parentPath = parent?.filename;
       const packageJsonPath = path.resolve(pkg.path, 'package.json');

diff --git a/lib/internal/modules/esm/package_config.js b/lib/internal/modules/esm/package_config.js
@@ -1,102 +1,10 @@
 'use strict';
 
 const {
-  JSONParse,
-  ObjectPrototypeHasOwnProperty,
-  SafeMap,
   StringPrototypeEndsWith,
 } = primordials;
 const { URL, fileURLToPath } = require('internal/url');
-const {
-  ERR_INVALID_PACKAGE_CONFIG,
-} = require('internal/errors').codes;
-
-const { filterOwnProperties } = require('internal/util');
-
-
-/**
- * @typedef {string | string[] | Record<string, unknown>} Exports
- * @typedef {'module' | 'commonjs'} PackageType
- * @typedef {{
- *   pjsonPath: string,
- *   exports?: ExportConfig,
- *   name?: string,
- *   main?: string,
- *   type?: PackageType,
- * }} PackageConfig
- */
-
-/** @type {Map<string, PackageConfig>} */
-const packageJSONCache = new SafeMap();
-
-
-/**
- * @param {string} path
- * @param {string} specifier
- * @param {string | URL | undefined} base
- * @returns {PackageConfig}
- */
-function getPackageConfig(path, specifier, base) {
-  const existing = packageJSONCache.get(path);
-  if (existing !== undefined) {
-    return existing;
-  }
-  const packageJsonReader = require('internal/modules/package_json_reader');
-  const source = packageJsonReader.read(path).string;
-  if (source === undefined) {
-    const packageConfig = {
-      pjsonPath: path,
-      exists: false,
-      main: undefined,
-      name: undefined,
-      type: 'none',
-      exports: undefined,
-      imports: undefined,
-    };
-    packageJSONCache.set(path, packageConfig);
-    return packageConfig;
-  }
-
-  let packageJSON;
-  try {
-    packageJSON = JSONParse(source);
-  } catch (error) {
-    throw new ERR_INVALID_PACKAGE_CONFIG(
-      path,
-      (base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier),
-      error.message,
-    );
-  }
-
-  let { imports, main, name, type } = filterOwnProperties(packageJSON, ['imports', 'main', 'name', 'type']);
-  const exports = ObjectPrototypeHasOwnProperty(packageJSON, 'exports') ? packageJSON.exports : undefined;
-  if (typeof imports !== 'object' || imports === null) {
-    imports = undefined;
-  }
-  if (typeof main !== 'string') {
-    main = undefined;
-  }
-  if (typeof name !== 'string') {
-    name = undefined;
-  }
-  // Ignore unknown types for forwards compatibility
-  if (type !== 'module' && type !== 'commonjs') {
-    type = 'none';
-  }
-
-  const packageConfig = {
-    pjsonPath: path,
-    exists: true,
-    main,
-    name,
-    type,
-    exports,
-    imports,
-  };
-  packageJSONCache.set(path, packageConfig);
-  return packageConfig;
-}
-
+const packageJsonReader = require('internal/modules/package_json_reader');
 
 /**
  * @param {URL | string} resolved
@@ -109,7 +17,7 @@ function getPackageScopeConfig(resolved) {
     if (StringPrototypeEndsWith(packageJSONPath, 'node_modules/package.json')) {
       break;
     }
-    const packageConfig = getPackageConfig(fileURLToPath(packageJSONUrl), resolved);
+    const packageConfig = packageJsonReader.read(fileURLToPath(packageJSONUrl), { specifier: resolved, isESM: true });
     if (packageConfig.exists) {
       return packageConfig;
     }
@@ -124,7 +32,7 @@ function getPackageScopeConfig(resolved) {
     }
   }
   const packageJSONPath = fileURLToPath(packageJSONUrl);
-  const packageConfig = {
+  return {
     pjsonPath: packageJSONPath,
     exists: false,
     main: undefined,
@@ -133,12 +41,9 @@ function getPackageScopeConfig(resolved) {
     exports: undefined,
     imports: undefined,
   };
-  packageJSONCache.set(packageJSONPath, packageConfig);
-  return packageConfig;
 }
 
 
 module.exports = {
-  getPackageConfig,
   getPackageScopeConfig,
 };
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -53,8 +53,9 @@ const {
 } = require('internal/errors').codes;
 
 const { Module: CJSModule } = require('internal/modules/cjs/loader');
-const { getPackageConfig, getPackageScopeConfig } = require('internal/modules/esm/package_config');
+const { getPackageScopeConfig } = require('internal/modules/esm/package_config');
 const { getConditionsSet } = require('internal/modules/esm/utils');
+const packageJsonReader = require('internal/modules/package_json_reader');
 const { internalModuleStat } = internalBinding('fs');
 
 /**
@@ -734,8 +735,7 @@ function packageResolve(specifier, base, conditions) {
   const packageConfig = getPackageScopeConfig(base);
   if (packageConfig.exists) {
     const packageJSONUrl = pathToFileURL(packageConfig.pjsonPath);
-    if (packageConfig.name === packageName &&
-        packageConfig.exports !== undefined && packageConfig.exports !== null) {
+    if (packageConfig.exports != null && packageConfig.name === packageName) {
       return packageExportsResolve(
         packageJSONUrl, packageSubpath, packageConfig, base, conditions);
     }
@@ -759,8 +759,8 @@ function packageResolve(specifier, base, conditions) {
     }
 
     // Package match.
-    const packageConfig = getPackageConfig(packageJSONPath, specifier, base);
-    if (packageConfig.exports !== undefined && packageConfig.exports !== null) {
+    const packageConfig = packageJsonReader.read(packageJSONPath, { specifier, base, isESM: true });
+    if (packageConfig.exports != null) {
       return packageExportsResolve(
         packageJSONUrl, packageSubpath, packageConfig, base, conditions);
     }

diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js
@@ -1,30 +1,106 @@
 'use strict';
 
-const { SafeMap } = primordials;
+const {
+  JSONParse,
+  ObjectPrototypeHasOwnProperty,
+  SafeMap,
+} = primordials;
+const {
+  ERR_INVALID_PACKAGE_CONFIG,
+} = require('internal/errors').codes;
 const { internalModuleReadJSON } = internalBinding('fs');
-const { pathToFileURL } = require('url');
 const { toNamespacedPath } = require('path');
+const { kEmptyObject } = require('internal/util');
+const { fileURLToPath, pathToFileURL } = require('internal/url');
 
 const cache = new SafeMap();
 
 let manifest;
 
 /**
- *
+ * @typedef {{
+ *   exists: boolean,
+ *   pjsonPath: string,
+ *   exports?: string | string[] | Record<string, unknown>,
+ *   imports?: string | string[] | Record<string, unknown>,
+ *   name?: string,
+ *   main?: string,
+ *   type: 'commonjs' | 'module' | 'none',
+ * }} PackageConfig
+ */
+
+/**
  * @param {string} jsonPath
+ * @param {{
+ *   base?: string,
+ *   specifier: string,
+ *   isESM: boolean,
+ * }} options
+ * @returns {PackageConfig}
  */
-function read(jsonPath) {
+function read(jsonPath, { base, specifier, isESM } = kEmptyObject) {
   if (cache.has(jsonPath)) {
     return cache.get(jsonPath);
   }
 
-  const { 0: string, 1: containsKeys } = internalModuleReadJSON(
+  const string = internalModuleReadJSON(
     toNamespacedPath(jsonPath),
   );
-  const result = { string, containsKeys };
-  const { getOptionValue } = require('internal/options');
+  const result = {
+    __proto__: null,
+    exists: false,
+    pjsonPath: jsonPath,
+    main: undefined,
+    name: undefined,
+    type: 'none', // Ignore unknown types for forwards compatibility
+    exports: undefined,
+    imports: undefined,
+  };
+
   if (string !== undefined) {
+    let parsed;
+    try {
+      parsed = JSONParse(string);
+    } catch (error) {
+      if (isESM) {
+        throw new ERR_INVALID_PACKAGE_CONFIG(
+          jsonPath,
+          (base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier),
+          error.message,
+        );
+      } else {
+        error.message = 'Error parsing ' + jsonPath + ': ' + error.message;
+        error.path = jsonPath;
+        throw error;
+      }
+    }
+
+    result.exists = true;
+
+    // ObjectPrototypeHasOwnProperty is used to avoid prototype pollution.
+    if (ObjectPrototypeHasOwnProperty(parsed, 'name') && typeof parsed.name === 'string') {
+      result.name = parsed.name;
+    }
+
+    if (ObjectPrototypeHasOwnProperty(parsed, 'main') && typeof parsed.main === 'string') {
+      result.main = parsed.main;
+    }
+
+    if (ObjectPrototypeHasOwnProperty(parsed, 'exports')) {
+      result.exports = parsed.exports;
+    }
+
+    if (ObjectPrototypeHasOwnProperty(parsed, 'imports')) {
+      result.imports = parsed.imports;
+    }
+
+    // Ignore unknown types for forwards compatibility
+    if (ObjectPrototypeHasOwnProperty(parsed, 'type') && (parsed.type === 'commonjs' || parsed.type === 'module')) {
+      result.type = parsed.type;
+    }
+
     if (manifest === undefined) {
+      const { getOptionValue } = require('internal/options');
       manifest = getOptionValue('--experimental-policy') ?
         require('internal/process/policy').manifest :
         null;