From ab885a226eb05eb715df9da05c63c11731abf494 Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Wed, 20 Nov 2024 17:21:14 -0800 Subject: [PATCH] feat(compartment-mapper): Relax package discovery --- packages/compartment-mapper/NEWS.md | 15 ++++++++ packages/compartment-mapper/src/archive.js | 8 ++++ packages/compartment-mapper/src/import.js | 2 + .../compartment-mapper/src/node-modules.js | 33 ++++++++++++---- .../compartment-mapper/src/types/external.ts | 7 ++++ .../fixtures-0/node_modules/app/package.json | 3 +- .../fixtures-strict/node_modules/app/main.js | 1 + .../node_modules/app/package.json | 11 ++++++ packages/compartment-mapper/test/scaffold.js | 14 +++++++ .../compartment-mapper/test/strict.test.js | 38 +++++++++++++++++++ 10 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 packages/compartment-mapper/test/fixtures-strict/node_modules/app/main.js create mode 100644 packages/compartment-mapper/test/fixtures-strict/node_modules/app/package.json create mode 100644 packages/compartment-mapper/test/strict.test.js diff --git a/packages/compartment-mapper/NEWS.md b/packages/compartment-mapper/NEWS.md index 1a749a4d1e..5124764413 100644 --- a/packages/compartment-mapper/NEWS.md +++ b/packages/compartment-mapper/NEWS.md @@ -1,5 +1,20 @@ User-visible changes to `@endo/compartment-mapper`: +# Next release + +- `mapNodeModules` and all functions that use it now tolerate the absence of + expected packages. + These packages are now omitted from the generated package skeleton map. + So, loading a physically missing module now occurs during the load phase + instead of the mapping phase. +- Adds a `strict` option to all functions that `mapNodeModules` to restore old + behavior, which produces an error early if, for example, a non-optional + peer dependency is missing. + Peer dependencies are strictly required unless `peerDependenciesMeta` has an + object with a truthy `optional` entry. + Correct interpretation of `peerDependencies` is not distributed evenly, so + this behavior is no longer the default. + # v1.4.0 (2024-11-13) - Adds options `languageForExtension`, `moduleLanguageForExtension`, diff --git a/packages/compartment-mapper/src/archive.js b/packages/compartment-mapper/src/archive.js index b9b4af7837..b7535629df 100644 --- a/packages/compartment-mapper/src/archive.js +++ b/packages/compartment-mapper/src/archive.js @@ -85,6 +85,7 @@ export const makeArchive = async (powers, moduleLocation, options = {}) => { dev, tags, conditions = tags, + strict = false, commonDependencies, policy, languageForExtension, @@ -99,6 +100,7 @@ export const makeArchive = async (powers, moduleLocation, options = {}) => { } = assignParserForLanguage(options); const compartmentMap = await mapNodeModules(powers, moduleLocation, { dev, + strict, conditions, commonDependencies, policy, @@ -129,6 +131,7 @@ export const mapLocation = async (powers, moduleLocation, options = {}) => { dev, tags, conditions = tags, + strict = false, commonDependencies, policy, parserForLanguage, @@ -144,6 +147,7 @@ export const mapLocation = async (powers, moduleLocation, options = {}) => { const compartmentMap = await mapNodeModules(powers, moduleLocation, { dev, + strict, conditions, commonDependencies, policy, @@ -174,6 +178,7 @@ export const hashLocation = async (powers, moduleLocation, options = {}) => { dev, tags, conditions = tags, + strict = false, commonDependencies, policy, parserForLanguage, @@ -189,6 +194,7 @@ export const hashLocation = async (powers, moduleLocation, options = {}) => { const compartmentMap = await mapNodeModules(powers, moduleLocation, { dev, + strict, conditions, commonDependencies, policy, @@ -226,6 +232,7 @@ export const writeArchive = async ( dev, tags, conditions = tags, + strict = false, commonDependencies, policy, parserForLanguage, @@ -240,6 +247,7 @@ export const writeArchive = async ( } = assignParserForLanguage(options); const compartmentMap = await mapNodeModules(readPowers, moduleLocation, { dev, + strict, conditions, commonDependencies, policy, diff --git a/packages/compartment-mapper/src/import.js b/packages/compartment-mapper/src/import.js index 1c6c3cedef..00c9d02975 100644 --- a/packages/compartment-mapper/src/import.js +++ b/packages/compartment-mapper/src/import.js @@ -76,6 +76,7 @@ export const loadLocation = async ( const { dev, tags, + strict, commonDependencies, policy, parserForLanguage, @@ -93,6 +94,7 @@ export const loadLocation = async ( 'conditions' in options ? options.conditions || tags : tags; const compartmentMap = await mapNodeModules(readPowers, moduleLocation, { dev, + strict, conditions, commonDependencies, policy, diff --git a/packages/compartment-mapper/src/node-modules.js b/packages/compartment-mapper/src/node-modules.js index d2582a75a0..ef1f183d00 100644 --- a/packages/compartment-mapper/src/node-modules.js +++ b/packages/compartment-mapper/src/node-modules.js @@ -296,6 +296,7 @@ const inferParsers = (descriptor, location, languageOptions) => { * @param {boolean | undefined} dev * @param {CommonDependencyDescriptors} commonDependencyDescriptors * @param {LanguageOptions} languageOptions + * @param {boolean} strict * @param {Map>} preferredPackageLogicalPathMap * @param {Array} logicalPath * @returns {Promise} @@ -310,6 +311,7 @@ const graphPackage = async ( dev, commonDependencyDescriptors, languageOptions, + strict, preferredPackageLogicalPathMap = new Map(), logicalPath = [], ) => { @@ -344,14 +346,18 @@ const graphPackage = async ( devDependencies = {}, } = packageDescriptor; const allDependencies = {}; - assign(allDependencies, commonDependencyDescriptors); - for (const [name, { spec }] of Object.entries(commonDependencyDescriptors)) { - allDependencies[name] = spec; + for (const [name, descriptor] of Object.entries( + commonDependencyDescriptors, + )) { + if (Object(descriptor) === descriptor) { + const { spec } = descriptor; + allDependencies[name] = spec; + } } assign(allDependencies, dependencies); assign(allDependencies, peerDependencies); - for (const [name, { optional }] of Object.entries(peerDependenciesMeta)) { - if (optional) { + for (const [name, meta] of Object.entries(peerDependenciesMeta)) { + if (Object(meta) === meta && meta.optional) { optionals.add(name); } } @@ -380,6 +386,7 @@ const graphPackage = async ( conditions, preferredPackageLogicalPathMap, languageOptions, + strict, childLogicalPath, optional, commonDependencyDescriptors, @@ -481,6 +488,7 @@ const graphPackage = async ( * @param {Set} conditions * @param {Map>} preferredPackageLogicalPathMap * @param {LanguageOptions} languageOptions + * @param {boolean} strict * @param {Array} [childLogicalPath] * @param {boolean} [optional] - whether the dependency is optional * @param {object} [commonDependencyDescriptors] - dependencies to be added to all packages @@ -495,6 +503,7 @@ const gatherDependency = async ( conditions, preferredPackageLogicalPathMap, languageOptions, + strict, childLogicalPath = [], optional = false, commonDependencyDescriptors = undefined, @@ -507,7 +516,7 @@ const gatherDependency = async ( ); if (dependency === undefined) { // allow the dependency to be missing if optional - if (optional) { + if (optional || !strict) { return; } throw Error(`Cannot find dependency ${name} for ${packageLocation}`); @@ -532,6 +541,7 @@ const gatherDependency = async ( false, commonDependencyDescriptors, languageOptions, + strict, preferredPackageLogicalPathMap, childLogicalPath, ); @@ -556,6 +566,7 @@ const gatherDependency = async ( * only this package). * @param {Record} commonDependencies - dependencies to be added to all packages * @param {LanguageOptions} languageOptions + * @param {boolean} strict */ const graphPackages = async ( maybeRead, @@ -566,6 +577,7 @@ const graphPackages = async ( dev, commonDependencies, languageOptions, + strict, ) => { const memo = create(null); /** @@ -623,6 +635,7 @@ const graphPackages = async ( dev, commonDependencyDescriptors, languageOptions, + strict, ); return graph; }; @@ -863,7 +876,12 @@ export const compartmentMapForNodeModules = async ( moduleSpecifier, options = {}, ) => { - const { dev = false, commonDependencies = {}, policy } = options; + const { + dev = false, + commonDependencies = {}, + policy, + strict = false, + } = options; const { maybeRead, canonical } = unpackReadPowers(readPowers); const languageOptions = makeLanguageOptions(options); @@ -883,6 +901,7 @@ export const compartmentMapForNodeModules = async ( dev || (conditions && conditions.has('development')), commonDependencies, languageOptions, + strict, ); if (policy) { diff --git a/packages/compartment-mapper/src/types/external.ts b/packages/compartment-mapper/src/types/external.ts index d0de4e9916..7216547568 100644 --- a/packages/compartment-mapper/src/types/external.ts +++ b/packages/compartment-mapper/src/types/external.ts @@ -67,6 +67,13 @@ type MapNodeModulesOptionsOmitPolicy = Partial<{ * of having the `"development"` condition. */ dev: boolean; + /** + * Indicates that the node_modules tree should fail to map if it does not + * strictly reach every expected package. + * By default, unreachable packages are simply omitted from the map, + * which defers some errors to when modules load. + */ + strict: boolean; /** Dependencies to make reachable from any package */ commonDependencies: Record; /** Maps extensions to languages for all packages, like `txt` to `text` */ diff --git a/packages/compartment-mapper/test/fixtures-0/node_modules/app/package.json b/packages/compartment-mapper/test/fixtures-0/node_modules/app/package.json index f2cb18a3c6..811e047d2e 100644 --- a/packages/compartment-mapper/test/fixtures-0/node_modules/app/package.json +++ b/packages/compartment-mapper/test/fixtures-0/node_modules/app/package.json @@ -8,7 +8,8 @@ "brooke": "^1.0.0", "clarke": "^1.0.0", "danny": "^1.0.0", - "esmcjs": "^1.0.0" + "esmcjs": "^1.0.0", + "no-such-package": "^1.0.0" }, "devDependencies": { "typecommon": "^1.0.0", diff --git a/packages/compartment-mapper/test/fixtures-strict/node_modules/app/main.js b/packages/compartment-mapper/test/fixtures-strict/node_modules/app/main.js new file mode 100644 index 0000000000..7a4e8a723a --- /dev/null +++ b/packages/compartment-mapper/test/fixtures-strict/node_modules/app/main.js @@ -0,0 +1 @@ +export default 42; diff --git a/packages/compartment-mapper/test/fixtures-strict/node_modules/app/package.json b/packages/compartment-mapper/test/fixtures-strict/node_modules/app/package.json new file mode 100644 index 0000000000..9f21ffd8c0 --- /dev/null +++ b/packages/compartment-mapper/test/fixtures-strict/node_modules/app/package.json @@ -0,0 +1,11 @@ +{ + "name": "app", + "version": "1.0.0", + "type": "module", + "dependencies": { + "no-such-package": "^1.0.0" + }, + "scripts": { + "preinstall": "echo DO NOT INSTALL TEST FIXTURES; exit -1" + } +} diff --git a/packages/compartment-mapper/test/scaffold.js b/packages/compartment-mapper/test/scaffold.js index 00ee4ea127..aec454b739 100644 --- a/packages/compartment-mapper/test/scaffold.js +++ b/packages/compartment-mapper/test/scaffold.js @@ -95,6 +95,7 @@ export function scaffold( knownFailure = false, tags = undefined, conditions = tags, + strict = false, searchSuffixes = undefined, commonDependencies = undefined, parserForLanguage = undefined, @@ -154,6 +155,7 @@ export function scaffold( workspaceLanguageForExtension, workspaceCommonjsLanguageForExtension, workspaceModuleLanguageForExtension, + strict, ...additionalOptions, }); const { namespace } = await application.import({ @@ -182,6 +184,7 @@ export function scaffold( modules, Compartment, conditions: new Set(['development', ...(conditions || [])]), + strict, commonDependencies, languageForExtension, commonjsLanguageForExtension, @@ -226,6 +229,7 @@ export function scaffold( modules, Compartment, conditions: new Set(['development', ...(conditions || [])]), + strict, commonDependencies, languageForExtension, commonjsLanguageForExtension, @@ -268,6 +272,7 @@ export function scaffold( modules, Compartment, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, parserForLanguage, @@ -292,6 +297,7 @@ export function scaffold( modules, policy, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, parserForLanguage, @@ -337,6 +343,7 @@ export function scaffold( modules, policy, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, parserForLanguage, @@ -390,6 +397,7 @@ export function scaffold( modules: { builtin: true }, policy, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, parserForLanguage, @@ -451,6 +459,7 @@ export function scaffold( policy, modules, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, sourceMapHook, @@ -500,6 +509,7 @@ export function scaffold( const map = await mapNodeModules(readPowers, fixture, { policy, conditions: new Set(['development', ...(conditions || [])]), + strict, commonDependencies, languages, languageForExtension, @@ -553,6 +563,7 @@ export function scaffold( modules, Compartment, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, parserForLanguage, @@ -568,6 +579,7 @@ export function scaffold( const archiveBytes = await makeArchive(readPowers, fixture, { modules, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, parserForLanguage, @@ -605,6 +617,7 @@ export function scaffold( modules, Compartment, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, parserForLanguage, @@ -620,6 +633,7 @@ export function scaffold( const archive = await makeArchive(readPowers, fixture, { modules, conditions: new Set(['development', ...(conditions || [])]), + strict, searchSuffixes, commonDependencies, parserForLanguage, diff --git a/packages/compartment-mapper/test/strict.test.js b/packages/compartment-mapper/test/strict.test.js new file mode 100644 index 0000000000..b60f3d49a2 --- /dev/null +++ b/packages/compartment-mapper/test/strict.test.js @@ -0,0 +1,38 @@ +import 'ses'; +import test from 'ava'; +import { scaffold } from './scaffold.js'; + +// The JSONP parser uses harden, as a bit. +lockdown({ + errorTaming: 'unsafe', + errorTrapping: 'none', +}); + +const assertions = (t, { namespace }) => { + t.is(namespace.default, 42); +}; + +const assertionCount = 1; + +scaffold( + 'not strict', + test, + new URL('fixtures-strict/node_modules/app/main.js', import.meta.url).href, + assertions, + assertionCount, +); + +scaffold( + 'strict', + test, + new URL('fixtures-strict/node_modules/app/main.js', import.meta.url).href, + assertions, + assertionCount, + { + strict: true, + knownFailure: true, + onError(t, error) { + t.ok(error.message.contains('Cannot find dependency no-such-package')); + }, + }, +);