chore: rename things, cleanup & and more tests

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,13 +629,20 @@ export function makeImportNowHookMaker(
         }
       }
 
-      const record = dynamicHook(moduleSpecifier, packageLocation);
+      if (exitModuleImportNowHook) {
+        const record = exitModuleImportNowHook(
+          moduleSpecifier,
+          packageLocation,
+        );
 
-      if (!record) {
-        throw new Error(`Could not import module: ${moduleSpecifier}`);
+        if (!record) {
+          throw new Error(`Could not import module: ${moduleSpecifier}`);
+        }
+
+        return record;
       }
 
-      return record;
+      throw new Error(`Could not import module: ${moduleSpecifier}`);
     };
 
     return importNowHook;

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,22 +34,35 @@ 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<Application>}
  */
 
 /**
  * @overload
  * @param {ReadFn | ReadPowers} readPowers
  * @param {CompartmentMapDescriptor} compartmentMap
- * @param {ImportLocationOptions} [options]
+ * @param {ImportLocationOptions} [opts]
  * @returns {Promise<Application>}
  */
 
@@ -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<void>} */
     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,14 +188,19 @@ 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,
         languageForExtension,
         globals,
         transforms,
         moduleTransforms,
-        syncModuleTransforms,
         __shimTransforms__,
         Compartment,
       }));

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<SomeObject>} the object of the imported modules exported
  * names.
  */

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,
     });
   }
 

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';

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<string>} [searchSuffixes]
  * @property {Record<string, string>} [commonDependencies]
  * @property {SourceMapHook} [sourceMapHook]
- * @property {DynamicImportHook} dynamicHook
+ * @property {ExitModuleImportNowHook} [importNowHook]
  * @property {Record<string, ParserImplementation>} [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<string, ParserImplementation>} [parserForLanguage]
  * @property {LanguageForExtension} [languageForExtension]
- * @property {DynamicImportHook} [dynamicHook]
+ * @property {ExitModuleImportNowHook} [importNowHook]
  */
 
 /**