esm: share package.json cache between ESM and CJS loaders

add test for ERR_INVALID_PACKAGE_CONFIG error Refs: nodejs#30674
shackijj · May 10, 2020 · 0227b07 · 0227b07
1 parent 94e5b5c
commit 0227b07
Show file tree

Hide file tree

Showing 5 changed files with 113 additions and 124 deletions.
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -94,8 +94,10 @@ const {
 const { validateString } = require('internal/validators');
 const pendingDeprecation = getOptionValue('--pending-deprecation');
 
+const packageJsonCache = new SafeMap();
+
 module.exports = {
-  wrapSafe, Module, toRealPath, readPackageScope,
+  wrapSafe, Module, toRealPath, readPackageScope, readPackage, packageJsonCache,
   get hasLoadedAnyUserCJSModule() { return hasLoadedAnyUserCJSModule; }
 };
 
@@ -239,16 +241,22 @@ Module._debug = deprecate(debug, 'Module._debug is deprecated.', 'DEP0077');
 //   -> a
 //   -> a.<ext>
 //   -> a/index.<ext>
+/**
+ *
+ * @param {string} jsonPath
+ * @returns {string|undefined}
+ */
+function readJsonDefaultStrategy(jsonPath) {
+  return internalModuleReadJSON(path.toNamespacedPath(jsonPath));
+}
 
-const packageJsonCache = new SafeMap();
-
-function readPackage(requestPath) {
+function readPackage(requestPath, readJsonStrategy = readJsonDefaultStrategy) {
   const jsonPath = path.resolve(requestPath, 'package.json');
 
   const existing = packageJsonCache.get(jsonPath);
   if (existing !== undefined) return existing;
 
-  const json = internalModuleReadJSON(path.toNamespacedPath(jsonPath));
+  const json = readJsonStrategy(jsonPath);
   if (json === undefined) {
     packageJsonCache.set(jsonPath, false);
     return false;
@@ -276,7 +284,8 @@ function readPackage(requestPath) {
   }
 }
 
-function readPackageScope(checkPath) {
+function readPackageScope(
+  checkPath, readJsonStrategy = readJsonDefaultStrategy) {
   const rootSeparatorIndex = checkPath.indexOf(path.sep);
   let separatorIndex;
   while (
@@ -285,7 +294,7 @@ function readPackageScope(checkPath) {
     checkPath = checkPath.slice(0, separatorIndex);
     if (checkPath.endsWith(path.sep + 'node_modules'))
       return false;
-    const pjson = readPackage(checkPath);
+    const pjson = readPackage(checkPath, readJsonStrategy);
     if (pjson) return {
       path: checkPath,
       data: pjson

diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -4,7 +4,6 @@ const {
   ArrayIsArray,
   ArrayPrototypeJoin,
   ArrayPrototypeShift,
-  JSONParse,
   JSONStringify,
   ObjectFreeze,
   ObjectGetOwnPropertyNames,
@@ -25,17 +24,22 @@ const assert = require('internal/assert');
 const internalFS = require('internal/fs/utils');
 const { NativeModule } = require('internal/bootstrap/loaders');
 const {
-  closeSync,
-  fstatSync,
-  openSync,
-  readFileSync,
+  Module: CJSModule,
+  readPackageScope,
+  readPackage,
+  packageJsonCache
+} = require('internal/modules/cjs/loader');
+const {
   realpathSync,
   statSync,
   Stats,
+  openSync,
+  fstatSync,
+  readFileSync,
+  closeSync
 } = require('fs');
 const { getOptionValue } = require('internal/options');
-const { sep, relative } = require('path');
-const { Module: CJSModule } = require('internal/modules/cjs/loader');
+const { sep, relative, join } = require('path');
 const preserveSymlinks = getOptionValue('--preserve-symlinks');
 const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
 const typeFlag = getOptionValue('--input-type');
@@ -66,7 +70,6 @@ function getConditionsSet(conditions) {
 }
 
 const realpathCache = new SafeMap();
-const packageJSONCache = new SafeMap();  /* string -> PackageConfig */
 
 function tryStatSync(path) {
   try {
@@ -76,107 +79,6 @@ function tryStatSync(path) {
   }
 }
 
-function readIfFile(path) {
-  let fd;
-  try {
-    fd = openSync(path, 'r');
-  } catch {
-    return undefined;
-  }
-  try {
-    if (!fstatSync(fd).isFile()) return undefined;
-    return readFileSync(fd, 'utf8');
-  } finally {
-    closeSync(fd);
-  }
-}
-
-function getPackageConfig(path, base) {
-  const existing = packageJSONCache.get(path);
-  if (existing !== undefined) {
-    if (!existing.isValid) {
-      throw new ERR_INVALID_PACKAGE_CONFIG(path, fileURLToPath(base), false);
-    }
-    return existing;
-  }
-
-  const source = readIfFile(path);
-  if (source === undefined) {
-    const packageConfig = {
-      exists: false,
-      main: undefined,
-      name: undefined,
-      isValid: true,
-      type: 'none',
-      exports: undefined
-    };
-    packageJSONCache.set(path, packageConfig);
-    return packageConfig;
-  }
-
-  let packageJSON;
-  try {
-    packageJSON = JSONParse(source);
-  } catch {
-    const packageConfig = {
-      exists: true,
-      main: undefined,
-      name: undefined,
-      isValid: false,
-      type: 'none',
-      exports: undefined
-    };
-    packageJSONCache.set(path, packageConfig);
-    return packageConfig;
-  }
-
-  let { main, name, type } = packageJSON;
-  const { exports } = packageJSON;
-  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 = {
-    exists: true,
-    main,
-    name,
-    isValid: true,
-    type,
-    exports
-  };
-  packageJSONCache.set(path, packageConfig);
-  return packageConfig;
-}
-
-function getPackageScopeConfig(resolved, base) {
-  let packageJSONUrl = new URL('./package.json', resolved);
-  while (true) {
-    const packageJSONPath = packageJSONUrl.pathname;
-    if (StringPrototypeEndsWith(packageJSONPath, 'node_modules/package.json'))
-      break;
-    const packageConfig = getPackageConfig(fileURLToPath(packageJSONUrl), base);
-    if (packageConfig.exists) return packageConfig;
-
-    const lastPackageJSONUrl = packageJSONUrl;
-    packageJSONUrl = new URL('../package.json', packageJSONUrl);
-
-    // Terminates at root where ../package.json equals ../../package.json
-    // (can't just check "/package.json" for Windows support).
-    if (packageJSONUrl.pathname === lastPackageJSONUrl.pathname) break;
-  }
-  const packageConfig = {
-    exists: false,
-    main: undefined,
-    name: undefined,
-    isValid: true,
-    type: 'none',
-    exports: undefined
-  };
-  packageJSONCache.set(fileURLToPath(packageJSONUrl), packageConfig);
-  return packageConfig;
-}
-
 /*
  * Legacy CommonJS main resolution:
  * 1. let M = pkg_url + (json main field)
@@ -429,7 +331,7 @@ function isConditionalExportsMainSugar(exports, packageJSONUrl, base) {
 
 
 function packageMainResolve(packageJSONUrl, packageConfig, base, conditions) {
-  if (packageConfig.exists) {
+  if (packageConfig) {
     const exports = packageConfig.exports;
     if (exports !== undefined) {
       if (isConditionalExportsMainSugar(exports, packageJSONUrl, base)) {
@@ -506,9 +408,51 @@ function packageExportsResolve(
   throwExportsNotFound(packageSubpath, packageJSONUrl, base);
 }
 
+function readIfFile(path) {
+  let fd;
+  try {
+    fd = openSync(path, 'r');
+  } catch {
+    return undefined;
+  }
+  try {
+    if (!fstatSync(fd).isFile()) return undefined;
+    return readFileSync(fd, 'utf8');
+  } finally {
+    closeSync(fd);
+  }
+}
+
+function catchInvalidPackage(error, path) {
+  if (error instanceof SyntaxError) {
+    const packageJsonPath = join(path, 'package.json');
+    const message = StringPrototypeSlice(
+      error.message, ('Error parsing ' + packageJsonPath + ': ').length);
+    throw new ERR_INVALID_PACKAGE_CONFIG(path, message, true);
+  }
+  throw error;
+}
+
+function readPackageESM(path) {
+  try {
+    return readPackage(path, readIfFile);
+  } catch (error) {
+    catchInvalidPackage(error, path);
+  }
+}
+
+function readPackageScopeESM(url) {
+  const path = fileURLToPath(url);
+  try {
+    return readPackageScope(path, readIfFile);
+  } catch (error) {
+    catchInvalidPackage(error, path);
+  }
+}
+
 function getPackageType(url) {
-  const packageConfig = getPackageScopeConfig(url, url);
-  return packageConfig.type;
+  const packageConfig = readPackageScopeESM(url);
+  return packageConfig ? packageConfig.data.type : 'none';
 }
 
 /**
@@ -552,12 +496,13 @@ function packageResolve(specifier, base, conditions) {
     '' : '.' + StringPrototypeSlice(specifier, separatorIndex);
 
   // ResolveSelf
-  const packageConfig = getPackageScopeConfig(base, base);
-  if (packageConfig.exists) {
+  const packageScope = readPackageScopeESM(base);
+  if (packageScope !== false) {
+    const packageConfig = packageScope.data;
     // TODO(jkrems): Find a way to forward the pair/iterator already generated
     // while executing GetPackageScopeConfig
     let packageJSONUrl;
-    for (const [ filename, packageConfigCandidate ] of packageJSONCache) {
+    for (const [ filename, packageConfigCandidate ] of packageJsonCache) {
       if (packageConfig === packageConfigCandidate) {
         packageJSONUrl = pathToFileURL(filename);
         break;
@@ -595,13 +540,15 @@ function packageResolve(specifier, base, conditions) {
     }
 
     // Package match.
-    const packageConfig = getPackageConfig(packageJSONPath, base);
+    const packagePath = StringPrototypeSlice(
+      packageJSONPath, 0, packageJSONPath.length - 13);
+    const packageConfig = readPackageESM(packagePath);
     if (packageSubpath === './') {
       return new URL('./', packageJSONUrl);
     } else if (packageSubpath === '') {
       return packageMainResolve(packageJSONUrl, packageConfig, base,
                                 conditions);
-    } else if (packageConfig.exports !== undefined) {
+    } else if (packageConfig && packageConfig.exports !== undefined) {
       return packageExportsResolve(
         packageJSONUrl, packageSubpath, packageConfig, base, conditions);
     } else {

diff --git a/test/es-module/test-esm-invalid-pjson.js b/test/es-module/test-esm-invalid-pjson.js
@@ -0,0 +1,29 @@
+'use strict';
+
+const { mustCall, isWindows } = require('../common');
+const fixtures = require('../common/fixtures');
+const { spawn } = require('child_process');
+const { strictEqual, ok } = require('assert');
+
+const entry = fixtures.path('/es-modules/import-invalid-pjson.mjs');
+const invalidJson = fixtures.path('/node_modules/invalid-pjson/package.json');
+
+const child = spawn(process.execPath, [entry]);
+child.stderr.setEncoding('utf8');
+let stderr = '';
+child.stderr.on('data', (data) => {
+  stderr += data;
+});
+child.on('close', mustCall((code, signal) => {
+  strictEqual(code, 1);
+  strictEqual(signal, null);
+  ok(
+    stderr.includes(
+      [
+        '[ERR_INVALID_PACKAGE_CONFIG]: ',
+        `Invalid package config ${invalidJson}, `,
+        `Unexpected token } in JSON at position ${isWindows ? 16 : 14}`
+      ].join(''),
+    ),
+    stderr);
+}));
diff --git a/test/fixtures/es-modules/import-invalid-pjson.mjs b/test/fixtures/es-modules/import-invalid-pjson.mjs
@@ -0,0 +1 @@
+import 'invalid-pjson';
diff --git a/test/fixtures/node_modules/invalid-pjson/package.json b/test/fixtures/node_modules/invalid-pjson/package.json