From 5bef20c2fcd1728e764615d5231a01a483a1b4cf Mon Sep 17 00:00:00 2001 From: Kirill Shatskiy Date: Mon, 4 May 2020 12:50:42 +0300 Subject: [PATCH] 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 Reviewed-By: Geoffrey Booth --- lib/internal/modules/cjs/loader.js | 9 ++-- lib/internal/modules/esm/resolve.js | 52 +++++-------------- lib/internal/modules/package_json_reader.js | 23 ++++++++ node.gyp | 1 + src/node_file.cc | 36 ++++++------- test/es-module/test-esm-invalid-pjson.js | 29 +++++++++++ .../es-modules/import-invalid-pjson.mjs | 1 + .../node_modules/invalid-pjson/package.json | 3 ++ test/parallel/test-bootstrap-modules.js | 1 + test/parallel/test-module-binding.js | 31 +++++++++-- 10 files changed, 119 insertions(+), 67 deletions(-) create mode 100644 lib/internal/modules/package_json_reader.js create mode 100644 test/es-module/test-esm-invalid-pjson.js create mode 100644 test/fixtures/es-modules/import-invalid-pjson.mjs create mode 100644 test/fixtures/node_modules/invalid-pjson/package.json diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 35eabb55f2b2ea..325b9adde7aecb 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -60,10 +60,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, @@ -255,7 +253,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; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 29235a6785f1bc..987a139c6aae57 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/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 ? diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js new file mode 100644 index 00000000000000..066047b55eb9d8 --- /dev/null +++ b/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 }; diff --git a/node.gyp b/node.gyp index 2c1a26c28cc79f..19738496277b26 100644 --- a/node.gyp +++ b/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', diff --git a/src/node_file.cc b/src/node_file.cc index d10a695ff9e679..0defca8361b05c 100644 --- a/src/node_file.cc +++ b/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& 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& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -853,14 +852,16 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo& 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& 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(numchars) == kBlockSize); @@ -924,18 +926,16 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { } } - Local 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 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 diff --git a/test/es-module/test-esm-invalid-pjson.js b/test/es-module/test-esm-invalid-pjson.js new file mode 100644 index 00000000000000..83f4ad5baba4a7 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..61f4aa83458bac --- /dev/null +++ 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 new file mode 100644 index 00000000000000..5864c29c04a731 --- /dev/null +++ b/test/fixtures/node_modules/invalid-pjson/package.json @@ -0,0 +1,3 @@ +{ + "invalid" +} diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 5c719d803cf427..62775b9527998b 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/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', diff --git a/test/parallel/test-module-binding.js b/test/parallel/test-module-binding.js index d0e122605ddc00..a0fa0a038990f0 100644 --- a/test/parallel/test-module-binding.js +++ b/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); }