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

Refs: https://github.com/nodejs/node/issues/30674

PR-URL: https://github.com/nodejs/node/pull/33229
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

lib/internal/modules/cjs/loader.js

@@ -61,10 +61,8 @@ const fs = require('fs');
 const internalFS = require('internal/fs/utils');
 const path = require('path');
 const { emitWarningSync } = require('internal/process/warning');
-const {
-  internalModuleReadJSON,
-  internalModuleStat
-} = internalBinding('fs');
+const { internalModuleStat } = internalBinding('fs');
+const packageJsonReader = require('internal/modules/package_json_reader');
 const { safeGetenv } = internalBinding('credentials');
 const {
   makeRequireFunction,
@@ -269,7 +267,8 @@ function readPackage(requestPath) {
   const existing = packageJsonCache.get(jsonPath);
   if (existing !== undefined) return existing;
 
-  const json = internalModuleReadJSON(path.toNamespacedPath(jsonPath));
+  const result = packageJsonReader.read(path.toNamespacedPath(jsonPath));
+  const json = result.containsKeys === false ? '{}' : result.string;
   if (json === undefined) {
     packageJsonCache.set(jsonPath, false);
     return false;

lib/internal/modules/esm/resolve.js

@@ -26,10 +26,6 @@ const assert = require('internal/assert');
 const internalFS = require('internal/fs/utils');
 const { NativeModule } = require('internal/bootstrap/loaders');
 const {
-  closeSync,
-  fstatSync,
-  openSync,
-  readFileSync,
   realpathSync,
   statSync,
   Stats,
@@ -53,6 +49,7 @@ const {
   ERR_UNSUPPORTED_ESM_URL_SCHEME,
 } = require('internal/errors').codes;
 
+const packageJsonReader = require('internal/modules/package_json_reader');
 const DEFAULT_CONDITIONS = ObjectFreeze(['node', 'import']);
 const DEFAULT_CONDITIONS_SET = new SafeSet(DEFAULT_CONDITIONS);
 
@@ -78,37 +75,25 @@ 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);
-  }
+/**
+ *
+ * '/foo/package.json' -> '/foo'
+ */
+function removePackageJsonFromPath(path) {
+  return StringPrototypeSlice(path, 0, path.length - 13);
 }
 
-function getPackageConfig(path, base) {
+function getPackageConfig(path) {
   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);
+  const source = packageJsonReader.read(path).string;
   if (source === undefined) {
     const packageConfig = {
       exists: false,
       main: undefined,
       name: undefined,
-      isValid: true,
       type: 'none',
       exports: undefined
     };
@@ -119,17 +104,9 @@ function getPackageConfig(path, base) {
   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;
+  } catch (error) {
+    const errorPath = removePackageJsonFromPath(path);
+    throw new ERR_INVALID_PACKAGE_CONFIG(errorPath, error.message, true);
   }
 
   let { main, name, type } = packageJSON;
@@ -143,7 +120,6 @@ function getPackageConfig(path, base) {
     exists: true,
     main,
     name,
-    isValid: true,
     type,
     exports
   };
@@ -171,7 +147,6 @@ function getPackageScopeConfig(resolved, base) {
     exists: false,
     main: undefined,
     name: undefined,
-    isValid: true,
     type: 'none',
     exports: undefined
   };
@@ -588,8 +563,7 @@ function packageResolve(specifier, base, conditions) {
   let packageJSONPath = fileURLToPath(packageJSONUrl);
   let lastPath;
   do {
-    const stat = tryStatSync(
-      StringPrototypeSlice(packageJSONPath, 0, packageJSONPath.length - 13));
+    const stat = tryStatSync(removePackageJsonFromPath(packageJSONPath));
     if (!stat.isDirectory()) {
       lastPath = packageJSONPath;
       packageJSONUrl = new URL((isScoped ?

lib/internal/modules/package_json_reader.js

@@ -0,0 +1,23 @@
+'use strict';
+
+const { SafeMap } = primordials;
+const { internalModuleReadJSON } = internalBinding('fs');
+
+const cache = new SafeMap();
+
+/**
+ *
+ * @param {string} path
+ */
+function read(path) {
+  if (cache.has(path)) {
+    return cache.get(path);
+  }
+
+  const [string, containsKeys] = internalModuleReadJSON(path);
+  const result = { string, containsKeys };
+  cache.set(path, result);
+  return result;
+}
+
+module.exports = { read };

node.gyp

@@ -159,6 +159,7 @@
       'lib/internal/main/run_third_party_main.js',
       'lib/internal/main/worker_thread.js',
       'lib/internal/modules/run_main.js',
+      'lib/internal/modules/package_json_reader.js',
       'lib/internal/modules/cjs/helpers.js',
       'lib/internal/modules/cjs/loader.js',
       'lib/internal/modules/esm/loader.js',

src/node_file.cc

@@ -51,6 +51,7 @@ namespace node {
 namespace fs {
 
 using v8::Array;
+using v8::Boolean;
 using v8::Context;
 using v8::EscapableHandleScope;
 using v8::Function;
@@ -842,9 +843,7 @@ void Close(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-// Used to speed up module loading.  Returns the contents of the file as
-// a string or undefined when the file cannot be opened or "main" is not found
-// in the file.
+// Used to speed up module loading. Returns an array [string, boolean]
 static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = env->isolate();
@@ -853,14 +852,16 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsString());
   node::Utf8Value path(isolate, args[0]);
 
-  if (strlen(*path) != path.length())
+  if (strlen(*path) != path.length()) {
+    args.GetReturnValue().Set(Array::New(isolate));
     return;  // Contains a nul byte.
-
+  }
   uv_fs_t open_req;
   const int fd = uv_fs_open(loop, &open_req, *path, O_RDONLY, 0, nullptr);
   uv_fs_req_cleanup(&open_req);
 
   if (fd < 0) {
+    args.GetReturnValue().Set(Array::New(isolate));
     return;
   }
 
@@ -886,9 +887,10 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
     numchars = uv_fs_read(loop, &read_req, fd, &buf, 1, offset, nullptr);
     uv_fs_req_cleanup(&read_req);
 
-    if (numchars < 0)
+    if (numchars < 0) {
+      args.GetReturnValue().Set(Array::New(isolate));
       return;
-
+    }
     offset += numchars;
   } while (static_cast<size_t>(numchars) == kBlockSize);
 
@@ -924,18 +926,16 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
     }
   }
 
-  Local<String> return_value;
-  if (p < pe) {
-    return_value =
-        String::NewFromUtf8(isolate,
-                            &chars[start],
-                            v8::NewStringType::kNormal,
-                            size).ToLocalChecked();
-  } else {
-    return_value = env->empty_object_string();
-  }
 
-  args.GetReturnValue().Set(return_value);
+  Local<Value> return_value[] = {
+    String::NewFromUtf8(isolate,
+                        &chars[start],
+                        v8::NewStringType::kNormal,
+                        size).ToLocalChecked(),
+    Boolean::New(isolate, p < pe ? true : false)
+  };
+  args.GetReturnValue().Set(
+    Array::New(isolate, return_value, arraysize(return_value)));
 }
 
 // Used to speed up module loading.  Returns 0 if the path refers to

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);
+}));

test/fixtures/es-modules/import-invalid-pjson.mjs

@@ -0,0 +1 @@
+import 'invalid-pjson';

test/fixtures/node_modules/invalid-pjson/package.json

@@ -0,0 +1,3 @@
+{
+  "invalid"
+}

test/parallel/test-bootstrap-modules.js

@@ -48,6 +48,7 @@ const expectedModules = new Set([
   'NativeModule internal/idna',
   'NativeModule internal/linkedlist',
   'NativeModule internal/modules/run_main',
+  'NativeModule internal/modules/package_json_reader',
   'NativeModule internal/modules/cjs/helpers',
   'NativeModule internal/modules/cjs/loader',
   'NativeModule internal/modules/esm/create_dynamic_module',

test/parallel/test-module-binding.js

@@ -6,11 +6,32 @@ const { internalBinding } = require('internal/test/binding');
 const { internalModuleReadJSON } = internalBinding('fs');
 const { readFileSync } = require('fs');
 const { strictEqual } = require('assert');
-
-strictEqual(internalModuleReadJSON('nosuchfile'), undefined);
-strictEqual(internalModuleReadJSON(fixtures.path('empty.txt')), '{}');
-strictEqual(internalModuleReadJSON(fixtures.path('empty-with-bom.txt')), '{}');
+{
+  const [string, containsKeys] = internalModuleReadJSON('nosuchfile');
+  strictEqual(string, undefined);
+  strictEqual(containsKeys, undefined);
+}
+{
+  const [string, containsKeys] =
+    internalModuleReadJSON(fixtures.path('empty.txt'));
+  strictEqual(string, '');
+  strictEqual(containsKeys, false);
+}
+{
+  const [string, containsKeys] =
+    internalModuleReadJSON(fixtures.path('empty.txt'));
+  strictEqual(string, '');
+  strictEqual(containsKeys, false);
+}
+{
+  const [string, containsKeys] =
+    internalModuleReadJSON(fixtures.path('empty-with-bom.txt'));
+  strictEqual(string, '');
+  strictEqual(containsKeys, false);
+}
 {
   const filename = fixtures.path('require-bin/package.json');
-  strictEqual(internalModuleReadJSON(filename), readFileSync(filename, 'utf8'));
+  const [string, containsKeys] = internalModuleReadJSON(filename);
+  strictEqual(string, readFileSync(filename, 'utf8'));
+  strictEqual(containsKeys, true);
 }