From 0227b07661a7b48b5f1c55147fbf5414b3460e93 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 add test for ERR_INVALID_PACKAGE_CONFIG error Refs: https://github.com/nodejs/node/issues/30674 --- lib/internal/modules/cjs/loader.js | 23 ++- lib/internal/modules/esm/resolve.js | 181 +++++++----------- test/es-module/test-esm-invalid-pjson.js | 29 +++ .../es-modules/import-invalid-pjson.mjs | 1 + .../node_modules/invalid-pjson/package.json | 3 + 5 files changed, 113 insertions(+), 124 deletions(-) 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 f2715d07cb44fb..27d4cb15e77615 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -94,8 +94,10 @@ const { const { validateString } = require('internal/validators'); const pendingDeprecation = getOptionValue('--pending-deprecation'); +const packageJsonCache = new SafeMap(); + module.exports = { - wrapSafe, Module, toRealPath, readPackageScope, + wrapSafe, Module, toRealPath, readPackageScope, readPackage, packageJsonCache, get hasLoadedAnyUserCJSModule() { return hasLoadedAnyUserCJSModule; } }; @@ -239,16 +241,22 @@ Module._debug = deprecate(debug, 'Module._debug is deprecated.', 'DEP0077'); // -> a // -> a. // -> a/index. +/** + * + * @param {string} jsonPath + * @returns {string|undefined} + */ +function readJsonDefaultStrategy(jsonPath) { + return internalModuleReadJSON(path.toNamespacedPath(jsonPath)); +} -const packageJsonCache = new SafeMap(); - -function readPackage(requestPath) { +function readPackage(requestPath, readJsonStrategy = readJsonDefaultStrategy) { const jsonPath = path.resolve(requestPath, 'package.json'); const existing = packageJsonCache.get(jsonPath); if (existing !== undefined) return existing; - const json = internalModuleReadJSON(path.toNamespacedPath(jsonPath)); + const json = readJsonStrategy(jsonPath); if (json === undefined) { packageJsonCache.set(jsonPath, false); return false; @@ -276,7 +284,8 @@ function readPackage(requestPath) { } } -function readPackageScope(checkPath) { +function readPackageScope( + checkPath, readJsonStrategy = readJsonDefaultStrategy) { const rootSeparatorIndex = checkPath.indexOf(path.sep); let separatorIndex; while ( @@ -285,7 +294,7 @@ function readPackageScope(checkPath) { checkPath = checkPath.slice(0, separatorIndex); if (checkPath.endsWith(path.sep + 'node_modules')) return false; - const pjson = readPackage(checkPath); + const pjson = readPackage(checkPath, readJsonStrategy); if (pjson) return { path: checkPath, data: pjson diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index c66e0554ad2746..a8f9af117c5bd1 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -4,7 +4,6 @@ const { ArrayIsArray, ArrayPrototypeJoin, ArrayPrototypeShift, - JSONParse, JSONStringify, ObjectFreeze, ObjectGetOwnPropertyNames, @@ -25,17 +24,22 @@ const assert = require('internal/assert'); const internalFS = require('internal/fs/utils'); const { NativeModule } = require('internal/bootstrap/loaders'); const { - closeSync, - fstatSync, - openSync, - readFileSync, + Module: CJSModule, + readPackageScope, + readPackage, + packageJsonCache +} = require('internal/modules/cjs/loader'); +const { realpathSync, statSync, Stats, + openSync, + fstatSync, + readFileSync, + closeSync } = require('fs'); const { getOptionValue } = require('internal/options'); -const { sep, relative } = require('path'); -const { Module: CJSModule } = require('internal/modules/cjs/loader'); +const { sep, relative, join } = require('path'); const preserveSymlinks = getOptionValue('--preserve-symlinks'); const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); const typeFlag = getOptionValue('--input-type'); @@ -66,7 +70,6 @@ function getConditionsSet(conditions) { } const realpathCache = new SafeMap(); -const packageJSONCache = new SafeMap(); /* string -> PackageConfig */ function tryStatSync(path) { try { @@ -76,107 +79,6 @@ 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) { - 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); - if (source === undefined) { - const packageConfig = { - exists: false, - main: undefined, - name: undefined, - isValid: true, - type: 'none', - exports: undefined - }; - packageJSONCache.set(path, packageConfig); - return packageConfig; - } - - 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; - } - - let { main, name, type } = packageJSON; - const { exports } = packageJSON; - if (typeof main !== 'string') main = undefined; - if (typeof name !== 'string') name = undefined; - // Ignore unknown types for forwards compatibility - if (type !== 'module' && type !== 'commonjs') type = 'none'; - - const packageConfig = { - exists: true, - main, - name, - isValid: true, - type, - exports - }; - packageJSONCache.set(path, packageConfig); - return packageConfig; -} - -function getPackageScopeConfig(resolved, base) { - let packageJSONUrl = new URL('./package.json', resolved); - while (true) { - const packageJSONPath = packageJSONUrl.pathname; - if (StringPrototypeEndsWith(packageJSONPath, 'node_modules/package.json')) - break; - const packageConfig = getPackageConfig(fileURLToPath(packageJSONUrl), base); - if (packageConfig.exists) return packageConfig; - - const lastPackageJSONUrl = packageJSONUrl; - packageJSONUrl = new URL('../package.json', packageJSONUrl); - - // Terminates at root where ../package.json equals ../../package.json - // (can't just check "/package.json" for Windows support). - if (packageJSONUrl.pathname === lastPackageJSONUrl.pathname) break; - } - const packageConfig = { - exists: false, - main: undefined, - name: undefined, - isValid: true, - type: 'none', - exports: undefined - }; - packageJSONCache.set(fileURLToPath(packageJSONUrl), packageConfig); - return packageConfig; -} - /* * Legacy CommonJS main resolution: * 1. let M = pkg_url + (json main field) @@ -429,7 +331,7 @@ function isConditionalExportsMainSugar(exports, packageJSONUrl, base) { function packageMainResolve(packageJSONUrl, packageConfig, base, conditions) { - if (packageConfig.exists) { + if (packageConfig) { const exports = packageConfig.exports; if (exports !== undefined) { if (isConditionalExportsMainSugar(exports, packageJSONUrl, base)) { @@ -506,9 +408,51 @@ function packageExportsResolve( throwExportsNotFound(packageSubpath, packageJSONUrl, base); } +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 catchInvalidPackage(error, path) { + if (error instanceof SyntaxError) { + const packageJsonPath = join(path, 'package.json'); + const message = StringPrototypeSlice( + error.message, ('Error parsing ' + packageJsonPath + ': ').length); + throw new ERR_INVALID_PACKAGE_CONFIG(path, message, true); + } + throw error; +} + +function readPackageESM(path) { + try { + return readPackage(path, readIfFile); + } catch (error) { + catchInvalidPackage(error, path); + } +} + +function readPackageScopeESM(url) { + const path = fileURLToPath(url); + try { + return readPackageScope(path, readIfFile); + } catch (error) { + catchInvalidPackage(error, path); + } +} + function getPackageType(url) { - const packageConfig = getPackageScopeConfig(url, url); - return packageConfig.type; + const packageConfig = readPackageScopeESM(url); + return packageConfig ? packageConfig.data.type : 'none'; } /** @@ -552,12 +496,13 @@ function packageResolve(specifier, base, conditions) { '' : '.' + StringPrototypeSlice(specifier, separatorIndex); // ResolveSelf - const packageConfig = getPackageScopeConfig(base, base); - if (packageConfig.exists) { + const packageScope = readPackageScopeESM(base); + if (packageScope !== false) { + const packageConfig = packageScope.data; // TODO(jkrems): Find a way to forward the pair/iterator already generated // while executing GetPackageScopeConfig let packageJSONUrl; - for (const [ filename, packageConfigCandidate ] of packageJSONCache) { + for (const [ filename, packageConfigCandidate ] of packageJsonCache) { if (packageConfig === packageConfigCandidate) { packageJSONUrl = pathToFileURL(filename); break; @@ -595,13 +540,15 @@ function packageResolve(specifier, base, conditions) { } // Package match. - const packageConfig = getPackageConfig(packageJSONPath, base); + const packagePath = StringPrototypeSlice( + packageJSONPath, 0, packageJSONPath.length - 13); + const packageConfig = readPackageESM(packagePath); if (packageSubpath === './') { return new URL('./', packageJSONUrl); } else if (packageSubpath === '') { return packageMainResolve(packageJSONUrl, packageConfig, base, conditions); - } else if (packageConfig.exports !== undefined) { + } else if (packageConfig && packageConfig.exports !== undefined) { return packageExportsResolve( packageJSONUrl, packageSubpath, packageConfig, base, conditions); } else { 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..559374345e71bf --- /dev/null +++ b/test/fixtures/node_modules/invalid-pjson/package.json @@ -0,0 +1,3 @@ +{ + "invalid" +} \ No newline at end of file