From 44ea8db2e63c323d5617285525a471be56c9370f Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Tue, 18 Jun 2024 13:36:44 -0700 Subject: [PATCH] chore: rename things, cleanup & and more tests --- .../compartment-mapper/src/import-hook.js | 6 +- .../compartment-mapper/src/import-lite.js | 85 ++++++--- packages/compartment-mapper/src/import.js | 4 +- packages/compartment-mapper/src/link.js | 23 ++- packages/compartment-mapper/src/powers.js | 17 ++ packages/compartment-mapper/src/types.js | 19 +- .../test/dynamic-require.test.js | 163 +++++++++++++----- 7 files changed, 237 insertions(+), 80 deletions(-) diff --git a/packages/compartment-mapper/src/import-hook.js b/packages/compartment-mapper/src/import-hook.js index 79480f3949..1fa0354324 100644 --- a/packages/compartment-mapper/src/import-hook.js +++ b/packages/compartment-mapper/src/import-hook.js @@ -423,7 +423,7 @@ export function makeImportNowHookMaker( computeSha512 = undefined, searchSuffixes = nodejsConventionSearchSuffixes, sourceMapHook = undefined, - dynamicHook, + exitModuleImportNowHook, }, ) { // Set of specifiers for modules (scoped to compartment) whose parser is not @@ -548,7 +548,7 @@ export function makeImportNowHookMaker( moduleBytes = readSync(moduleLocation); } catch (err) { if (err && err.code === 'ENOENT') { - // might be an exit module. use the fallback `dynamicHook` to import it + // might be an exit module. use the fallback `exitModuleImportNowHook` to import it // eslint-disable-next-line no-continue continue; } @@ -629,7 +629,7 @@ export function makeImportNowHookMaker( } } - const record = dynamicHook(moduleSpecifier, packageLocation); + const record = exitModuleImportNowHook(moduleSpecifier, packageLocation); if (!record) { throw new Error(`Could not import module: ${moduleSpecifier}`); diff --git a/packages/compartment-mapper/src/import-lite.js b/packages/compartment-mapper/src/import-lite.js index f315432076..a766f89184 100644 --- a/packages/compartment-mapper/src/import-lite.js +++ b/packages/compartment-mapper/src/import-lite.js @@ -16,11 +16,13 @@ // @ts-check /* eslint no-shadow: "off" */ - -/** @import {ArchiveOptions, CompartmentMapDescriptor, ImportLocationSyncOptions, ImportNowHookMaker, LoadArchiveOptions, ModuleTransforms, SyncArchiveOptions, SyncReadPowers} from './types.js' */ +/** @import {CompartmentMapDescriptor} from './types.js' */ +/** @import {SyncImportLocationOptions} from './types.js' */ +/** @import {ImportNowHookMaker} from './types.js' */ +/** @import {ModuleTransforms} from './types.js' */ +/** @import {SyncReadPowers} from './types.js' */ /** @import {Application} from './types.js' */ /** @import {ImportLocationOptions} from './types.js' */ -/** @import {LoadLocationOptions} from './types.js' */ /** @import {ExecuteFn} from './types.js' */ /** @import {ReadFn} from './types.js' */ /** @import {ReadPowers} from './types.js' */ @@ -32,14 +34,27 @@ import { makeImportHookMaker, makeImportNowHookMaker, } from './import-hook.js'; +import { isSyncReadPowers } from './powers.js'; const { assign, create, freeze } = Object; +/** + * Returns `true` if `value` is a {@link SyncImportLocationOptions}. + * + * The only requirement here is that `moduleTransforms` is _not_ present in + * `value`; `importNowHook` is optional. + * + * @param {ImportLocationOptions|SyncImportLocationOptions} value + * @returns {value is SyncImportLocationOptions} + */ +const isSyncOptions = value => + !value || (typeof value === 'object' && !('moduleTransforms' in value)); + /** * @overload * @param {SyncReadPowers} readPowers * @param {CompartmentMapDescriptor} compartmentMap - * @param {ImportLocationSyncOptions} options + * @param {SyncImportLocationOptions} [opts] * @returns {Promise} */ @@ -47,7 +62,7 @@ const { assign, create, freeze } = Object; * @overload * @param {ReadFn | ReadPowers} readPowers * @param {CompartmentMapDescriptor} compartmentMap - * @param {ImportLocationOptions} [options] + * @param {ImportLocationOptions} [opts] * @returns {Promise} */ @@ -60,7 +75,6 @@ const { assign, create, freeze } = Object; export const loadFromMap = async (readPowers, compartmentMap, options = {}) => { const { - syncModuleTransforms = {}, searchSuffixes = undefined, parserForLanguage: parserForLanguageOption = {}, languageForExtension: languageForExtensionOption = {}, @@ -74,23 +88,43 @@ export const loadFromMap = async (readPowers, compartmentMap, options = {}) => { ); /** - * This type guard determines which of the two paths through the code is taken. + * Object containing options and read powers which fulfills all requirements + * for creation of a {@link ImportNowHookMaker}, thus enabling dynamic import * - * If `options` is `SyncArchiveOptions`, we will permit dynamic requires. By definition, this must not include async module transforms, and must have a non-empty `dynamicHook` - * - * If `options` isn't `SyncArchiveOptions`, then no. + * @typedef SyncBehavior + * @property {SyncReadPowers} readPowers + * @property {SyncImportLocationOptions} options + * @property {'SYNC'} type + */ + + /** + * Object containing options and read powers which is incompatible with + * creation of a {@link ImportNowHookMaker}, thus disabling dynamic import * - * @param {ArchiveOptions|SyncArchiveOptions} value - * @returns {value is SyncArchiveOptions} + * @typedef AsyncBehavior + * @property {ReadFn|ReadPowers} readPowers + * @property {ImportLocationOptions} options + * @property {'ASYNC'} type */ - const isSyncOptions = value => 'dynamicHook' in value; - const moduleTransforms = isSyncOptions(options) - ? undefined - : /** @type {ModuleTransforms} */ ({ - ...syncModuleTransforms, - ...(options.moduleTransforms || {}), - }); + /** + * When we must control flow based on _n_ type guards consdering _n_ discrete + * values, grouping the values into an object, then leveraging a discriminated + * union (the `type` property) is one way to approach the problem. + */ + const behavior = + isSyncReadPowers(readPowers) && isSyncOptions(options) + ? /** @type {SyncBehavior} */ ({ + readPowers, + options: options || {}, + type: 'SYNC', + }) + : /** @type {AsyncBehavior} */ ({ + readPowers, + options: options || {}, + type: 'ASYNC', + }); + const { entry: { compartment: entryCompartmentName, module: entryModuleSpecifier }, } = compartmentMap; @@ -130,14 +164,16 @@ export const loadFromMap = async (readPowers, compartmentMap, options = {}) => { /** @type {Promise} */ let pendingJobsPromise; - if (isSyncOptions(options)) { + if (behavior.type === 'SYNC') { + const { importNowHook: exitModuleImportNowHook, syncModuleTransforms } = + behavior.options; makeImportNowHook = makeImportNowHookMaker( /** @type {SyncReadPowers} */ (readPowers), entryCompartmentName, { compartmentDescriptors: compartmentMap.compartments, searchSuffixes, - dynamicHook: options.dynamicHook, + exitModuleImportNowHook, }, ); ({ compartment, pendingJobsPromise } = link(compartmentMap, { @@ -152,6 +188,12 @@ export const loadFromMap = async (readPowers, compartmentMap, options = {}) => { Compartment, })); } else { + // sync module transforms are allowed, because they are "compatible" + // with async module transforms (not vice-versa) + const moduleTransforms = /** @type {ModuleTransforms} */ ({ + ...behavior.options.syncModuleTransforms, + ...behavior.options.moduleTransforms, + }); ({ compartment, pendingJobsPromise } = link(compartmentMap, { makeImportHook, parserForLanguage, @@ -159,7 +201,6 @@ export const loadFromMap = async (readPowers, compartmentMap, options = {}) => { globals, transforms, moduleTransforms, - syncModuleTransforms, __shimTransforms__, Compartment, })); diff --git a/packages/compartment-mapper/src/import.js b/packages/compartment-mapper/src/import.js index b18212a820..23fd313775 100644 --- a/packages/compartment-mapper/src/import.js +++ b/packages/compartment-mapper/src/import.js @@ -22,7 +22,7 @@ const { assign, create, freeze } = Object; /** @import {ImportLocationOptions} from './types.js' */ /** @import {SyncArchiveOptions} from './types.js' */ /** @import {LoadLocationOptions} from './types.js' */ -/** @import {ImportLocationSyncOptions} from './types.js' */ +/** @import {SyncImportLocationOptions} from './types.js' */ /** @import {SomeObject} from './types.js' */ /** @import {SyncReadPowers} from './types.js' */ /** @import {ArchiveOptions} from './types.js' */ @@ -89,7 +89,7 @@ export const loadLocation = async ( * @overload * @param {SyncReadPowers} readPowers * @param {string} moduleLocation - * @param {ImportLocationSyncOptions} options + * @param {SyncImportLocationOptions} options * @returns {Promise} the object of the imported modules exported * names. */ diff --git a/packages/compartment-mapper/src/link.js b/packages/compartment-mapper/src/link.js index 859707b350..8b13ee2dfa 100644 --- a/packages/compartment-mapper/src/link.js +++ b/packages/compartment-mapper/src/link.js @@ -443,6 +443,18 @@ const makeModuleMapHook = ( return moduleMapHook; }; +/** + * Returns `true` if `value` is a {@link SyncLinkOptions}. + * + * The only requirement here is that `moduleTransforms` is _not_ present in + * `value`; `makeImportNowHook` is optional. + * + * @param {LinkOptions|SyncLinkOptions} value + * @returns {value is SyncLinkOptions} + */ +const isSyncOptions = value => + !value || (typeof value === 'object' && !('moduleTransforms' in value)); + /** * Assemble a DAG of compartments as declared in a compartment map starting at * the named compartment and building all compartments that it depends upon, @@ -520,16 +532,19 @@ export const link = ( /** @type {SyncModuleTransforms|undefined} */ let syncModuleTransforms; - const isSync = 'makeImportNowHook' in options; + const isSync = isSyncOptions(options); if (isSync) { makeImportNowHook = options.makeImportNowHook; syncModuleTransforms = options.syncModuleTransforms; } else { - const opts = /** @type {LinkOptions} */ (options); + // combine both sync and async module transforms + // if we're using dynamic requires. + // these MAY or MAY not have already been combined by + // the time this function is called! moduleTransforms = /** @type {ModuleTransforms} */ ({ - ...(opts.syncModuleTransforms || {}), - ...(opts.moduleTransforms || {}), + ...options.syncModuleTransforms, + ...options.moduleTransforms, }); } diff --git a/packages/compartment-mapper/src/powers.js b/packages/compartment-mapper/src/powers.js index 4e9d8f39ff..9707833525 100644 --- a/packages/compartment-mapper/src/powers.js +++ b/packages/compartment-mapper/src/powers.js @@ -8,6 +8,7 @@ // @ts-check /** @import {CanonicalFn} from './types.js' */ +/** @import {SyncReadPowers} from './types.js' */ /** @import {ReadFn} from './types.js' */ /** @import {ReadPowers} from './types.js' */ /** @import {MaybeReadPowers} from './types.js' */ @@ -51,3 +52,19 @@ export const unpackReadPowers = powers => { canonical, }; }; + +/** + * Returns `true` if `value` is a {@link SyncReadPowers}, which requires: + * + * 1. `readSync` is a function + * 2. `fileURLToPath` is a function + * + * @param {ReadPowers|ReadFn} value + * @returns {value is SyncReadPowers} + */ +export const isSyncReadPowers = value => + typeof value === 'object' && + 'readSync' in value && + typeof value.readSync === 'function' && + 'fileURLToPath' in value && + typeof value.fileURLToPath === 'function'; diff --git a/packages/compartment-mapper/src/types.js b/packages/compartment-mapper/src/types.js index ef3c41b080..6ff4dc925a 100644 --- a/packages/compartment-mapper/src/types.js +++ b/packages/compartment-mapper/src/types.js @@ -201,6 +201,8 @@ export {}; */ /** + * {@link ReadPowers} supporting synchronous reads and dynamic requires + * * @typedef {ReadPowers & {readSync: ReadSyncFn, fileURLToPath: FileURLToPathFn}} SyncReadPowers */ @@ -215,7 +217,7 @@ export {}; * Mapper lifts CommonJS up, more like a bundler, and does not attempt to vary * the behavior of resolution depending on the language of the importing module. * @property {SourceMapHook} [sourceMapHook] - * @property {DynamicImportHook} dynamicHook + * @property {ExitModuleImportNowHook} [exitModuleImportNowHook] */ /** @@ -372,9 +374,10 @@ export {}; */ /** - * @callback DynamicImportHook + * @callback ExitModuleImportNowHook * @param {string} specifier * @param {string} referrer + * @returns {ThirdPartyStaticModuleInterface|undefined} module namespace */ /** @@ -572,7 +575,7 @@ export {}; * @property {Array} [searchSuffixes] * @property {Record} [commonDependencies] * @property {SourceMapHook} [sourceMapHook] - * @property {DynamicImportHook} dynamicHook + * @property {ExitModuleImportNowHook} [importNowHook] * @property {Record} [parserForLanguage] * @property {LanguageForExtension} [languageForExtension] */ @@ -789,12 +792,14 @@ export {}; /** * Options for `importLocation()` * - * @typedef {ExecuteOptions & (ArchiveOptions|SyncArchiveOptions)} ImportLocationOptions + * @typedef {ExecuteOptions & ArchiveOptions} ImportLocationOptions */ /** - * Options for `importLocation()` for dynamic require support - * @typedef {ExecuteOptions & SyncArchiveOptions} ImportLocationSyncOptions + * Options for `importLocation()` necessary (but not sufficient--see + * {@link SyncReadPowers}) for dynamic require support + * + * @typedef {ExecuteOptions & SyncArchiveOptions} SyncImportLocationOptions */ /** @@ -813,7 +818,7 @@ export {}; * @property {SourceMapHook} [sourceMapHook] * @property {Record} [parserForLanguage] * @property {LanguageForExtension} [languageForExtension] - * @property {DynamicImportHook} [dynamicHook] + * @property {ExitModuleImportNowHook} [importNowHook] */ /** diff --git a/packages/compartment-mapper/test/dynamic-require.test.js b/packages/compartment-mapper/test/dynamic-require.test.js index c0fde5a254..9e81233cb8 100644 --- a/packages/compartment-mapper/test/dynamic-require.test.js +++ b/packages/compartment-mapper/test/dynamic-require.test.js @@ -1,5 +1,9 @@ // @ts-check /* eslint-disable import/no-dynamic-require */ + +/** @import {ExitModuleImportNowHook} from '../src/types.js' */ +/** @import {SyncModuleTransforms} from '../src/types.js' */ + import 'ses'; import test from 'ava'; import fs from 'node:fs'; @@ -16,27 +20,7 @@ test('intra-package dynamic require works', async t => { 'fixtures-dynamic/node_modules/app/index.js', import.meta.url, ).toString(); - let fallbackCalls = 0; - const { namespace } = await importLocation(readPowers, fixture, { - dynamicHook: (specifier, packageLocation) => { - fallbackCalls += 1; - const require = Module.createRequire( - readPowers.fileURLToPath(packageLocation), - ); - /** @type {object} */ - const ns = require(specifier); - return freeze( - /** @type {import('ses').ThirdPartyStaticModuleInterface} */ ({ - imports: [], - exports: keys(ns), - execute: moduleExports => { - moduleExports.default = ns; - assign(moduleExports, ns); - }, - }), - ); - }, - }); + const { namespace } = await importLocation(readPowers, fixture); t.deepEqual( { @@ -47,34 +31,100 @@ test('intra-package dynamic require works', async t => { }, { ...namespace }, ); - t.is(fallbackCalls, 0); }); -test('inter-pkg and builtin dynamic require works', async t => { +test('dynamic require fails without sync read powers', async t => { + const fixture = new URL( + 'fixtures-dynamic/node_modules/app/index.js', + import.meta.url, + ).toString(); + const { read } = readPowers; + await t.throwsAsync(importLocation(read, fixture), { + message: /Cannot find module '\.\/sprunt\.js'/, + }); +}); + +test('dynamic exit module loading fails without importNowHook', async t => { const fixture = new URL( 'fixtures-dynamic/node_modules/hooked-app/index.js', import.meta.url, ).toString(); - let fallbackCalls = 0; + + await t.throwsAsync(importLocation(readPowers, fixture), { + message: /Failed to load module "cluster"/, + }); +}); + +test('inter-pkg and exit module dynamic require works', async t => { + const fixture = new URL( + 'fixtures-dynamic/node_modules/hooked-app/index.js', + import.meta.url, + ).toString(); + + t.plan(2); + + // number of times the `importNowHook` got called + let importNowHookCallCount = 0; + + /** @type {ExitModuleImportNowHook} */ + const importNowHook = (specifier, packageLocation) => { + importNowHookCallCount += 1; + const require = Module.createRequire( + readPowers.fileURLToPath(packageLocation), + ); + /** @type {object} */ + const ns = require(specifier); + return freeze( + /** @type {import('ses').ThirdPartyStaticModuleInterface} */ ({ + imports: [], + exports: keys(ns), + execute: moduleExports => { + moduleExports.default = ns; + assign(moduleExports, ns); + }, + }), + ); + }; + const { namespace } = await importLocation(readPowers, fixture, { - dynamicHook: (specifier, packageLocation) => { - fallbackCalls += 1; - const require = Module.createRequire( - readPowers.fileURLToPath(packageLocation), - ); - /** @type {object} */ - const ns = require(specifier); - return freeze( - /** @type {import('ses').ThirdPartyStaticModuleInterface} */ ({ - imports: [], - exports: keys(ns), - execute: moduleExports => { - moduleExports.default = ns; - assign(moduleExports, ns); - }, - }), - ); + importNowHook, + }); + + t.deepEqual( + { + default: { + isOk: 1, + }, + isOk: 1, + }, + { ...namespace }, + ); + t.is(importNowHookCallCount, 1); +}); + +test('sync module transforms work with dynamic require support', async t => { + const fixture = new URL( + 'fixtures-dynamic/node_modules/app/index.js', + import.meta.url, + ).toString(); + + t.plan(2); + + let transformCount = 0; + + /** @type {SyncModuleTransforms} */ + const syncModuleTransforms = { + cjs: sourceBytes => { + transformCount += 1; + return { + bytes: sourceBytes, + parser: 'cjs', + }; }, + }; + + const { namespace } = await importLocation(readPowers, fixture, { + syncModuleTransforms, }); t.deepEqual( @@ -86,5 +136,34 @@ test('inter-pkg and builtin dynamic require works', async t => { }, { ...namespace }, ); - t.is(fallbackCalls, 1); + + t.true(transformCount > 0); +}); + +test('sync module transforms work without dynamic require support', async t => { + const fixture = new URL( + 'fixtures-cjs-compat/node_modules/app/index.js', + import.meta.url, + ).toString(); + + let transformCount = 0; + + /** @type {SyncModuleTransforms} */ + const syncModuleTransforms = { + cjs: sourceBytes => { + transformCount += 1; + return { + bytes: sourceBytes, + parser: 'cjs', + }; + }, + }; + + const { read } = readPowers; + // no readSync, so no dynamic import support + await importLocation(read, fixture, { + syncModuleTransforms, + }); + + t.true(transformCount > 0); });