esm: merge esm and cjs package.json caches

lib/internal/modules/cjs/loader.js

@@ -59,10 +59,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,
@@ -248,7 +246,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

@@ -25,10 +25,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,
@@ -51,6 +47,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);
 
@@ -76,37 +73,17 @@ 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) {
+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
     };
@@ -117,17 +94,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 = StringPrototypeSlice(path, 0, path.length - 13);
+    throw new ERR_INVALID_PACKAGE_CONFIG(errorPath, error.message, true);
   }
 
   let { main, name, type } = packageJSON;
@@ -141,7 +110,6 @@ function getPackageConfig(path, base) {
     exists: true,
     main,
     name,
-    isValid: true,
     type,
     exports
   };
@@ -169,7 +137,6 @@ function getPackageScopeConfig(resolved, base) {
     exists: false,
     main: undefined,
     name: undefined,
-    isValid: true,
     type: 'none',
     exports: undefined
   };

lib/internal/modules/package_json_reader.js

@@ -0,0 +1,22 @@
+'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 result = internalModuleReadJSON(path);
+  cache.set(path, result);
+  return result;
+}
+
+module.exports = { read };

node.gyp

@@ -158,6 +158,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;
@@ -831,25 +832,27 @@ 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 object
+// {string: string | undefined, containsKeys: undefined | boolean}
 static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = env->isolate();
   uv_loop_t* loop = env->event_loop();
+  Local<Object> return_value = Object::New(isolate);
 
   CHECK(args[0]->IsString());
   node::Utf8Value path(isolate, args[0]);
 
-  if (strlen(*path) != path.length())
+  if (strlen(*path) != path.length()) {
+    args.GetReturnValue().Set(return_value);
     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(return_value);
     return;
   }
 
@@ -875,9 +878,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(return_value);
       return;
-
+    }
     offset += numchars;
   } while (static_cast<size_t>(numchars) == kBlockSize);
 
@@ -912,18 +916,18 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
       if (0 == memcmp(s, "exports", 7)) break;
     }
   }
-
-  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();
-  }
-
+  return_value->Set(
+    isolate->GetCurrentContext(),
+    FIXED_ONE_BYTE_STRING(isolate, "string"),
+    String::NewFromUtf8(isolate,
+                    &chars[start],
+                    v8::NewStringType::kNormal,
+                    size).ToLocalChecked()).Check();
+
+  return_value->Set(
+    isolate->GetCurrentContext(),
+    FIXED_ONE_BYTE_STRING(isolate, "containsKeys"),
+    Boolean::New(isolate, p < pe ? true : false)).Check();
   args.GetReturnValue().Set(return_value);
 }
 

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/parallel/test-bootstrap-modules.js

@@ -49,6 +49,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,29 @@ 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 result = internalModuleReadJSON('nosuchfile');
+  strictEqual(result.string, undefined);
+  strictEqual(result.containsKeys, undefined);
+}
+{
+  const result = internalModuleReadJSON(fixtures.path('empty.txt'));
+  strictEqual(result.string, '');
+  strictEqual(result.containsKeys, false);
+}
+{
+  const result = internalModuleReadJSON(fixtures.path('empty.txt'));
+  strictEqual(result.string, '');
+  strictEqual(result.containsKeys, false);
+}
+{
+  const result = internalModuleReadJSON(fixtures.path('empty-with-bom.txt'));
+  strictEqual(result.string, '');
+  strictEqual(result.containsKeys, false);
+}
 {
   const filename = fixtures.path('require-bin/package.json');
-  strictEqual(internalModuleReadJSON(filename), readFileSync(filename, 'utf8'));
+  const result = internalModuleReadJSON(filename);
+  strictEqual(result.string, readFileSync(filename, 'utf8'));
+  strictEqual(result.containsKeys, true);
 }