Skip to content

Commit

Permalink
esm: share package.json cache between ESM and CJS loaders
Browse files Browse the repository at this point in the history
Refs: #30674

PR-URL: #33229
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
  • Loading branch information
shackijj authored and codebytere committed Jul 8, 2020
1 parent 10b88cb commit 3559471
Show file tree
Hide file tree
Showing 10 changed files with 119 additions and 67 deletions.
9 changes: 4 additions & 5 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,8 @@ const assert = require('internal/assert');
const fs = require('fs');
const internalFS = require('internal/fs/utils');
const path = require('path');
const {
internalModuleReadJSON,
internalModuleStat
} = internalBinding('fs');
const { internalModuleStat } = internalBinding('fs');
const packageJsonReader = require('internal/modules/package_json_reader');
const { safeGetenv } = internalBinding('credentials');
const {
makeRequireFunction,
Expand Down Expand Up @@ -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;
Expand Down
52 changes: 13 additions & 39 deletions lib/internal/modules/esm/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);

Expand All @@ -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
};
Expand All @@ -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;
Expand All @@ -143,7 +120,6 @@ function getPackageConfig(path, base) {
exists: true,
main,
name,
isValid: true,
type,
exports
};
Expand Down Expand Up @@ -171,7 +147,6 @@ function getPackageScopeConfig(resolved, base) {
exists: false,
main: undefined,
name: undefined,
isValid: true,
type: 'none',
exports: undefined
};
Expand Down Expand Up @@ -590,8 +565,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 ?
Expand Down
23 changes: 23 additions & 0 deletions lib/internal/modules/package_json_reader.js
Original file line number Diff line number Diff line change
@@ -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 };
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,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',
Expand Down
36 changes: 18 additions & 18 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ namespace node {
namespace fs {

using v8::Array;
using v8::Boolean;
using v8::Context;
using v8::EscapableHandleScope;
using v8::Function;
Expand Down Expand Up @@ -826,9 +827,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();
Expand All @@ -837,14 +836,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;
}

Expand All @@ -870,9 +871,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);

Expand Down Expand Up @@ -908,18 +910,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
Expand Down
29 changes: 29 additions & 0 deletions test/es-module/test-esm-invalid-pjson.js
Original file line number Diff line number Diff line change
@@ -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);
}));
1 change: 1 addition & 0 deletions test/fixtures/es-modules/import-invalid-pjson.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import 'invalid-pjson';
3 changes: 3 additions & 0 deletions test/fixtures/node_modules/invalid-pjson/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/parallel/test-bootstrap-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
31 changes: 26 additions & 5 deletions test/parallel/test-module-binding.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit 3559471

Please sign in to comment.