Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 24d52e6

Browse files
anonriganonrig
committedJun 16, 2023
lib: merge cjs and esm package json reader caches
1 parent 0d725d6 commit 24d52e6

File tree

7 files changed

+111
-203
lines changed

7 files changed

+111
-203
lines changed
 

‎lib/internal/modules/cjs/loader.js

+11-37
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@ const {
8282
pendingDeprecate,
8383
emitExperimentalWarning,
8484
kEmptyObject,
85-
filterOwnProperties,
8685
setOwnProperty,
8786
getLazy,
8887
} = require('internal/util');
@@ -353,36 +352,10 @@ function initializeCJS() {
353352
// -> a.<ext>
354353
// -> a/index.<ext>
355354

356-
const packageJsonCache = new SafeMap();
357-
358355
function readPackage(requestPath) {
359356
const jsonPath = path.resolve(requestPath, 'package.json');
360-
361-
const existing = packageJsonCache.get(jsonPath);
362-
if (existing !== undefined) return existing;
363-
364-
const result = packageJsonReader.read(jsonPath);
365-
const json = result.containsKeys === false ? '{}' : result.string;
366-
if (json === undefined) {
367-
packageJsonCache.set(jsonPath, false);
368-
return false;
369-
}
370-
371-
try {
372-
const filtered = filterOwnProperties(JSONParse(json), [
373-
'name',
374-
'main',
375-
'exports',
376-
'imports',
377-
'type',
378-
]);
379-
packageJsonCache.set(jsonPath, filtered);
380-
return filtered;
381-
} catch (e) {
382-
e.path = jsonPath;
383-
e.message = 'Error parsing ' + jsonPath + ': ' + e.message;
384-
throw e;
385-
}
357+
// Return undefined or the filtered package.json as a JS object
358+
return packageJsonReader.read(jsonPath);
386359
}
387360

388361
let _readPackage = readPackage;
@@ -412,7 +385,7 @@ function readPackageScope(checkPath) {
412385
if (StringPrototypeEndsWith(checkPath, sep + 'node_modules'))
413386
return false;
414387
const pjson = _readPackage(checkPath + sep);
415-
if (pjson) return {
388+
if (pjson.exists) return {
416389
data: pjson,
417390
path: checkPath,
418391
};
@@ -421,13 +394,13 @@ function readPackageScope(checkPath) {
421394
}
422395

423396
function tryPackage(requestPath, exts, isMain, originalPath) {
424-
const pkg = _readPackage(requestPath)?.main;
397+
const pkg = _readPackage(requestPath);
425398

426-
if (!pkg) {
399+
if (!pkg.main) {
427400
return tryExtensions(path.resolve(requestPath, 'index'), exts, isMain);
428401
}
429402

430-
const filename = path.resolve(requestPath, pkg);
403+
const filename = path.resolve(requestPath, pkg.main);
431404
let actual = tryFile(filename, isMain) ||
432405
tryExtensions(filename, exts, isMain) ||
433406
tryExtensions(path.resolve(filename, 'index'), exts, isMain);
@@ -447,7 +420,7 @@ function tryPackage(requestPath, exts, isMain, originalPath) {
447420
} else {
448421
const jsonPath = path.resolve(requestPath, 'package.json');
449422
process.emitWarning(
450-
`Invalid 'main' field in '${jsonPath}' of '${pkg}'. ` +
423+
`Invalid 'main' field in '${jsonPath}' of '${pkg.main}'. ` +
451424
'Please either fix that or report it to the module author',
452425
'DeprecationWarning',
453426
'DEP0128',
@@ -527,8 +500,9 @@ function trySelf(parentPath, request) {
527500
if (!parentPath) return false;
528501

529502
const { data: pkg, path: pkgPath } = readPackageScope(parentPath) || {};
530-
if (!pkg || pkg.exports === undefined) return false;
531-
if (typeof pkg.name !== 'string') return false;
503+
if (!pkg || pkg.exports === undefined || pkg.name === undefined) {
504+
return false;
505+
}
532506

533507
let expansion;
534508
if (request === pkg.name) {
@@ -563,7 +537,7 @@ function resolveExports(nmPath, request) {
563537
return;
564538
const pkgPath = path.resolve(nmPath, name);
565539
const pkg = _readPackage(pkgPath);
566-
if (pkg?.exports != null) {
540+
if (pkg.exists && pkg.exports != null) {
567541
try {
568542
const { packageExportsResolve } = require('internal/modules/esm/resolve');
569543
return finalizeEsmResolution(packageExportsResolve(

‎lib/internal/modules/esm/package_config.js

+3-86
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,10 @@
11
'use strict';
22

33
const {
4-
JSONParse,
5-
ObjectPrototypeHasOwnProperty,
6-
SafeMap,
74
StringPrototypeEndsWith,
85
} = primordials;
96
const { URL, fileURLToPath } = require('internal/url');
10-
const {
11-
ERR_INVALID_PACKAGE_CONFIG,
12-
} = require('internal/errors').codes;
13-
14-
const { filterOwnProperties } = require('internal/util');
15-
7+
const packageJsonReader = require('internal/modules/package_json_reader');
168

179
/**
1810
* @typedef {string | string[] | Record<string, unknown>} Exports
@@ -26,78 +18,6 @@ const { filterOwnProperties } = require('internal/util');
2618
* }} PackageConfig
2719
*/
2820

29-
/** @type {Map<string, PackageConfig>} */
30-
const packageJSONCache = new SafeMap();
31-
32-
33-
/**
34-
* @param {string} path
35-
* @param {string} specifier
36-
* @param {string | URL | undefined} base
37-
* @returns {PackageConfig}
38-
*/
39-
function getPackageConfig(path, specifier, base) {
40-
const existing = packageJSONCache.get(path);
41-
if (existing !== undefined) {
42-
return existing;
43-
}
44-
const packageJsonReader = require('internal/modules/package_json_reader');
45-
const source = packageJsonReader.read(path).string;
46-
if (source === undefined) {
47-
const packageConfig = {
48-
pjsonPath: path,
49-
exists: false,
50-
main: undefined,
51-
name: undefined,
52-
type: 'none',
53-
exports: undefined,
54-
imports: undefined,
55-
};
56-
packageJSONCache.set(path, packageConfig);
57-
return packageConfig;
58-
}
59-
60-
let packageJSON;
61-
try {
62-
packageJSON = JSONParse(source);
63-
} catch (error) {
64-
throw new ERR_INVALID_PACKAGE_CONFIG(
65-
path,
66-
(base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier),
67-
error.message,
68-
);
69-
}
70-
71-
let { imports, main, name, type } = filterOwnProperties(packageJSON, ['imports', 'main', 'name', 'type']);
72-
const exports = ObjectPrototypeHasOwnProperty(packageJSON, 'exports') ? packageJSON.exports : undefined;
73-
if (typeof imports !== 'object' || imports === null) {
74-
imports = undefined;
75-
}
76-
if (typeof main !== 'string') {
77-
main = undefined;
78-
}
79-
if (typeof name !== 'string') {
80-
name = undefined;
81-
}
82-
// Ignore unknown types for forwards compatibility
83-
if (type !== 'module' && type !== 'commonjs') {
84-
type = 'none';
85-
}
86-
87-
const packageConfig = {
88-
pjsonPath: path,
89-
exists: true,
90-
main,
91-
name,
92-
type,
93-
exports,
94-
imports,
95-
};
96-
packageJSONCache.set(path, packageConfig);
97-
return packageConfig;
98-
}
99-
100-
10121
/**
10222
* @param {URL | string} resolved
10323
* @returns {PackageConfig}
@@ -109,7 +29,7 @@ function getPackageScopeConfig(resolved) {
10929
if (StringPrototypeEndsWith(packageJSONPath, 'node_modules/package.json')) {
11030
break;
11131
}
112-
const packageConfig = getPackageConfig(fileURLToPath(packageJSONUrl), resolved);
32+
const packageConfig = packageJsonReader.read(fileURLToPath(packageJSONUrl), { specifier: resolved, isESM: true });
11333
if (packageConfig.exists) {
11434
return packageConfig;
11535
}
@@ -124,7 +44,7 @@ function getPackageScopeConfig(resolved) {
12444
}
12545
}
12646
const packageJSONPath = fileURLToPath(packageJSONUrl);
127-
const packageConfig = {
47+
return {
12848
pjsonPath: packageJSONPath,
12949
exists: false,
13050
main: undefined,
@@ -133,12 +53,9 @@ function getPackageScopeConfig(resolved) {
13353
exports: undefined,
13454
imports: undefined,
13555
};
136-
packageJSONCache.set(packageJSONPath, packageConfig);
137-
return packageConfig;
13856
}
13957

14058

14159
module.exports = {
142-
getPackageConfig,
14360
getPackageScopeConfig,
14461
};

‎lib/internal/modules/esm/resolve.js

+5-5
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,9 @@ const {
5353
} = require('internal/errors').codes;
5454

5555
const { Module: CJSModule } = require('internal/modules/cjs/loader');
56-
const { getPackageConfig, getPackageScopeConfig } = require('internal/modules/esm/package_config');
56+
const { getPackageScopeConfig } = require('internal/modules/esm/package_config');
5757
const { getConditionsSet } = require('internal/modules/esm/utils');
58+
const packageJsonReader = require('internal/modules/package_json_reader');
5859
const { internalModuleStat } = internalBinding('fs');
5960

6061
/**
@@ -734,8 +735,7 @@ function packageResolve(specifier, base, conditions) {
734735
const packageConfig = getPackageScopeConfig(base);
735736
if (packageConfig.exists) {
736737
const packageJSONUrl = pathToFileURL(packageConfig.pjsonPath);
737-
if (packageConfig.name === packageName &&
738-
packageConfig.exports !== undefined && packageConfig.exports !== null) {
738+
if (packageConfig.name === packageName && packageConfig.exports != null) {
739739
return packageExportsResolve(
740740
packageJSONUrl, packageSubpath, packageConfig, base, conditions);
741741
}
@@ -759,8 +759,8 @@ function packageResolve(specifier, base, conditions) {
759759
}
760760

761761
// Package match.
762-
const packageConfig = getPackageConfig(packageJSONPath, specifier, base);
763-
if (packageConfig.exports !== undefined && packageConfig.exports !== null) {
762+
const packageConfig = packageJsonReader.read(packageJSONPath, { specifier, base, isESM: true });
763+
if (packageConfig.exports != null) {
764764
return packageExportsResolve(
765765
packageJSONUrl, packageSubpath, packageConfig, base, conditions);
766766
}

‎lib/internal/modules/package_json_reader.js

+78-7
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,101 @@
11
'use strict';
22

3-
const { SafeMap } = primordials;
3+
const {
4+
JSONParse,
5+
ObjectPrototypeHasOwnProperty,
6+
SafeMap,
7+
} = primordials;
8+
const {
9+
ERR_INVALID_PACKAGE_CONFIG,
10+
} = require('internal/errors').codes;
411
const { internalModuleReadJSON } = internalBinding('fs');
5-
const { pathToFileURL } = require('url');
612
const { toNamespacedPath } = require('path');
13+
const { kEmptyObject } = require('internal/util');
14+
const { fileURLToPath, pathToFileURL } = require('internal/url');
715

816
const cache = new SafeMap();
917

1018
let manifest;
1119

1220
/**
13-
*
1421
* @param {string} jsonPath
22+
* @param {{
23+
* base?: string,
24+
* specifier: string,
25+
* isESM: boolean,
26+
* }} options
27+
* @returns {{
28+
* exists: boolean,
29+
* pjsonPath: string,
30+
* name?: string,
31+
* main?: string,
32+
* exports?: string | Record<string, unknown>,
33+
* imports?: string | Record<string, unknown>,
34+
* type: 'commonjs' | 'module' | 'none',
35+
* }}
1536
*/
16-
function read(jsonPath) {
37+
function read(jsonPath, { base, specifier, isESM } = kEmptyObject) {
1738
if (cache.has(jsonPath)) {
1839
return cache.get(jsonPath);
1940
}
2041

21-
const { 0: string, 1: containsKeys } = internalModuleReadJSON(
42+
const string = internalModuleReadJSON(
2243
toNamespacedPath(jsonPath),
2344
);
24-
const result = { string, containsKeys };
25-
const { getOptionValue } = require('internal/options');
45+
const result = {
46+
__proto__: null,
47+
exists: false,
48+
pjsonPath: jsonPath,
49+
main: undefined,
50+
name: undefined,
51+
type: 'none',
52+
exports: undefined,
53+
imports: undefined,
54+
};
55+
2656
if (string !== undefined) {
57+
let parsed;
58+
try {
59+
parsed = JSONParse(string);
60+
} catch (error) {
61+
if (isESM) {
62+
throw new ERR_INVALID_PACKAGE_CONFIG(
63+
jsonPath,
64+
(base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier),
65+
error.message,
66+
);
67+
} else {
68+
error.path = jsonPath;
69+
error.message = 'Error parsing ' + jsonPath + ': ' + error.message;
70+
throw error;
71+
}
72+
}
73+
74+
result.exists = true;
75+
76+
// ObjectPrototypeHasOwnProperty is used to avoid prototype pollution.
77+
if (ObjectPrototypeHasOwnProperty(parsed, 'name') && typeof parsed.name === 'string') {
78+
result.name = parsed.name;
79+
}
80+
81+
if (ObjectPrototypeHasOwnProperty(parsed, 'main') && typeof parsed.main === 'string') {
82+
result.main = parsed.main;
83+
}
84+
85+
if (ObjectPrototypeHasOwnProperty(parsed, 'exports')) {
86+
result.exports = parsed.exports;
87+
}
88+
89+
if (ObjectPrototypeHasOwnProperty(parsed, 'imports')) {
90+
result.imports = parsed.imports;
91+
}
92+
93+
if (ObjectPrototypeHasOwnProperty(parsed, 'type') && (parsed.type === 'commonjs' || parsed.type === 'module')) {
94+
result.type = parsed.type;
95+
}
96+
2797
if (manifest === undefined) {
98+
const { getOptionValue } = require('internal/options');
2899
manifest = getOptionValue('--experimental-policy') ?
29100
require('internal/process/policy').manifest :
30101
null;

‎src/node_file.cc

+6-40
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ namespace fs {
5454

5555
using v8::Array;
5656
using v8::BigInt;
57-
using v8::Boolean;
5857
using v8::Context;
5958
using v8::EscapableHandleScope;
6059
using v8::Function;
@@ -1032,15 +1031,15 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
10321031
env, permission::PermissionScope::kFileSystemRead, path.ToStringView());
10331032

10341033
if (strlen(*path) != path.length()) {
1035-
args.GetReturnValue().Set(Array::New(isolate));
1034+
args.GetReturnValue().Set(Undefined(isolate));
10361035
return; // Contains a nul byte.
10371036
}
10381037
uv_fs_t open_req;
10391038
const int fd = uv_fs_open(loop, &open_req, *path, O_RDONLY, 0, nullptr);
10401039
uv_fs_req_cleanup(&open_req);
10411040

10421041
if (fd < 0) {
1043-
args.GetReturnValue().Set(Array::New(isolate));
1042+
args.GetReturnValue().Set(Undefined(isolate));
10441043
return;
10451044
}
10461045

@@ -1067,7 +1066,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
10671066
uv_fs_req_cleanup(&read_req);
10681067

10691068
if (numchars < 0) {
1070-
args.GetReturnValue().Set(Array::New(isolate));
1069+
args.GetReturnValue().Set(Undefined(isolate));
10711070
return;
10721071
}
10731072
offset += numchars;
@@ -1077,45 +1076,12 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
10771076
if (offset >= 3 && 0 == memcmp(chars.data(), "\xEF\xBB\xBF", 3)) {
10781077
start = 3; // Skip UTF-8 BOM.
10791078
}
1080-
10811079
const size_t size = offset - start;
1082-
char* p = &chars[start];
1083-
char* pe = &chars[size];
1084-
char* pos[2];
1085-
char** ppos = &pos[0];
1086-
1087-
while (p < pe) {
1088-
char c = *p++;
1089-
if (c == '\\' && p < pe && *p == '"') p++;
1090-
if (c != '"') continue;
1091-
*ppos++ = p;
1092-
if (ppos < &pos[2]) continue;
1093-
ppos = &pos[0];
1094-
1095-
char* s = &pos[0][0];
1096-
char* se = &pos[1][-1]; // Exclude quote.
1097-
size_t n = se - s;
1098-
1099-
if (n == 4) {
1100-
if (0 == memcmp(s, "main", 4)) break;
1101-
if (0 == memcmp(s, "name", 4)) break;
1102-
if (0 == memcmp(s, "type", 4)) break;
1103-
} else if (n == 7) {
1104-
if (0 == memcmp(s, "exports", 7)) break;
1105-
if (0 == memcmp(s, "imports", 7)) break;
1106-
}
1107-
}
11081080

1109-
1110-
Local<Value> return_value[] = {
1111-
String::NewFromUtf8(isolate,
1112-
&chars[start],
1113-
v8::NewStringType::kNormal,
1114-
size).ToLocalChecked(),
1115-
Boolean::New(isolate, p < pe ? true : false)
1116-
};
11171081
args.GetReturnValue().Set(
1118-
Array::New(isolate, return_value, arraysize(return_value)));
1082+
String::NewFromUtf8(
1083+
isolate, &chars[start], v8::NewStringType::kNormal, size)
1084+
.ToLocalChecked());
11191085
}
11201086

11211087
// Used to speed up module loading. Returns 0 if the path refers to

‎test/fixtures/errors/force_colors.snapshot

+4-4
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@ throw new Error('Should include grayed stack trace')
44

55
Error: Should include grayed stack trace
66
at Object.<anonymous> (/test*force_colors.js:1:7)
7-
 at Module._compile (node:internal*modules*cjs*loader:1255:14)
8-
 at Module._extensions..js (node:internal*modules*cjs*loader:1309:10)
9-
 at Module.load (node:internal*modules*cjs*loader:1113:32)
10-
 at Module._load (node:internal*modules*cjs*loader:960:12)
7+
 at Module._compile (node:internal*modules*cjs*loader:1229:14)
8+
 at Module._extensions..js (node:internal*modules*cjs*loader:1283:10)
9+
 at Module.load (node:internal*modules*cjs*loader:1087:32)
10+
 at Module._load (node:internal*modules*cjs*loader:934:12)
1111
 at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:83:12)
1212
 at node:internal*main*run_main_module:23:47
1313

‎test/parallel/test-module-binding.js

+4-24
Original file line numberDiff line numberDiff line change
@@ -4,34 +4,14 @@ require('../common');
44
const fixtures = require('../common/fixtures');
55
const { internalBinding } = require('internal/test/binding');
66
const { internalModuleReadJSON } = internalBinding('fs');
7-
const { readFileSync } = require('fs');
87
const { strictEqual } = require('assert');
8+
99
{
10-
const [string, containsKeys] = internalModuleReadJSON('nosuchfile');
11-
strictEqual(string, undefined);
12-
strictEqual(containsKeys, undefined);
10+
strictEqual(internalModuleReadJSON('nosuchfile'), undefined);
1311
}
1412
{
15-
const [string, containsKeys] =
16-
internalModuleReadJSON(fixtures.path('empty.txt'));
17-
strictEqual(string, '');
18-
strictEqual(containsKeys, false);
13+
strictEqual(internalModuleReadJSON(fixtures.path('empty.txt')), '');
1914
}
2015
{
21-
const [string, containsKeys] =
22-
internalModuleReadJSON(fixtures.path('empty.txt'));
23-
strictEqual(string, '');
24-
strictEqual(containsKeys, false);
25-
}
26-
{
27-
const [string, containsKeys] =
28-
internalModuleReadJSON(fixtures.path('empty-with-bom.txt'));
29-
strictEqual(string, '');
30-
strictEqual(containsKeys, false);
31-
}
32-
{
33-
const filename = fixtures.path('require-bin/package.json');
34-
const [string, containsKeys] = internalModuleReadJSON(filename);
35-
strictEqual(string, readFileSync(filename, 'utf8'));
36-
strictEqual(containsKeys, true);
16+
strictEqual(internalModuleReadJSON(fixtures.path('empty-with-bom.txt')), '');
3717
}

0 commit comments

Comments
 (0)
Please sign in to comment.