feat(compartment-mapper): expose experimental _redundantPreloadHook on captureFromMap

This adds a new hook option, `_redundantPreloadHook`, which is fired if and only if:

1. A canonical name w/ optional entry is present in the `_preload` array, _and_
2. The corresponding `Compartment` has already had its module sources loaded indirectly via loading the entry `Compartment`.

This better enables the caller to report the information to the end-user.

## Fixes

This change also fixes two bugs:

- The presence of `ModuleSource`s for a given `Compartment` does not imply _all_ preloaded modules have been loaded, as `_preload` may contain multiple entries for the same canonical name. It now checks individual entries.
- The type of `_preload` was incorrect; it demanded either an `Array<string>` _or_ an `Array<{name: string, entry: string}>`, when it should be possible to mix-and-match `string` and `{name: string, entry: string}` in the same array.

## Notes

- We could remove the `log()` call in `capture-lite.js`'s L187-L189 in lieu of the hook.
- Interested in any suggestions to avoid the assumption in `capture-lite.js`'s L181-L191; module resolution seems like a non-starter at this point, but maybe the mapping from `.` to a relative path exists in scope here already?

Author: boneskull

.changeset/good-jeans-fetch.md

@@ -0,0 +1,9 @@
+---
+'@endo/compartment-mapper': minor
+---
+
+Expose `_redundantPreloadHook` option in `captureFromMap()`, which will be called for each item in the `_preload` array that was already indirectly loaded via the entry `Compartment`.
+
+Fixes a bug in the type of `_preload` option, which now allows for mixed arrays.
+
+Fixes a bug in the preloader, which was not exhaustively checking if a non-entry module was already loaded via the entry `Compartment`.

packages/compartment-mapper/src/capture-lite.js

@@ -112,7 +112,12 @@ const captureCompartmentMap = (
 const makePreloader = (
   compartmentMap,
   sources,
-  { log = noop, policy, _preload: preload = [] } = {},
+  {
+    log = noop,
+    policy,
+    _preload: preload = [],
+    _redundantPreloadHook: redundantPreloadHook = undefined,
+  } = {},
 ) => {
   const {
     entry: { module: entryModuleSpecifier },
@@ -173,10 +178,29 @@ const makePreloader = (
 
         const compartmentSources = sources[compartmentName];
 
-        if (keys(compartmentSources).length) {
+        // The default preload entry is the entry module as defined by the
+        // package itself. This corresponds to the `ModuleConfiguration` of `.`
+        // in the `CompartmentDescriptor`'s `modules` object. Since the key in
+        // `compartmentSources` is presumably a resolved relative path, we don't
+        // actually know which key to look for! Thus, we are assuming that the
+        // `Compartment`'s entry module has been loaded if _any_ sources are
+        // present.
+        const entryIsLoaded =
+          entry === DEFAULT_PRELOAD_ENTRY
+            ? keys(compartmentSources).length > 0
+            : entry in compartmentSources;
+
+        if (entryIsLoaded) {
           log(
-            `Refusing to preload Compartment ${q(canonicalName)}; already loaded`,
+            `Refusing to preload Compartment ${q(canonicalName)} entry ${q(entry)}; already loaded`,
           );
+          if (redundantPreloadHook) {
+            redundantPreloadHook({
+              canonicalName,
+              entry,
+              log,
+            });
+          }
         } else {
           const compartment = compartments[compartmentName];
           if (!compartment) {
@@ -274,6 +298,7 @@ export const captureFromMap = async (
     Compartment: CompartmentOption = DefaultCompartment,
     log = noop,
     _preload: preload = [],
+    _redundantPreloadHook: redundantPreloadHook = undefined,
     packageConnectionsHook,
     moduleSourceHook,
   } = options;
@@ -295,6 +320,7 @@ export const captureFromMap = async (
     log,
     policy,
     _preload: preload,
+    _redundantPreloadHook: redundantPreloadHook,
   });
 
   const consolidatedExitModuleImportHook = exitModuleImportHookMaker({

packages/compartment-mapper/src/types/external.ts

@@ -141,6 +141,16 @@ export type PackageConnectionsHook = (params: {
   log: LogFn;
 }) => void;
 
+/**
+ * Hook executed during preloading when a compartment designated to be preloaded
+ * is already loaded.
+ */
+export type RedundantPreloadHook = (params: {
+  canonicalName: CanonicalName;
+  entry: string;
+  log: LogFn;
+}) => void;
+
 /**
  * Set of options available in the context of code execution.
  *
@@ -291,7 +301,13 @@ export interface PreloadOption {
    *
    * If an array of strings is provided, the entry is assumed to be `.`.
    */
-  _preload?: Array<string> | Array<{ compartment: string; entry: string }>;
+  _preload?: Array<string | { compartment: string; entry: string }>;
+
+  /**
+   * Hook executed during preloading when a compartment designated to be preloaded
+   * has already been loaded (via entry Compartment).
+   */
+  _redundantPreloadHook?: RedundantPreloadHook | undefined;
 }
 
 export type ArchiveLiteOptions = SyncOrAsyncArchiveOptions &

packages/compartment-mapper/src/types/internal.ts

@@ -41,6 +41,7 @@ import type {
   Sources,
   SyncModuleTransforms,
   CompartmentsRenameFn,
+  RedundantPreloadHook,
 } from './external.js';
 import type { PackageDescriptor } from './node-modules.js';
 import type { DeferredAttenuatorsProvider } from './policy.js';

packages/compartment-mapper/test/capture-lite.test.js

@@ -16,10 +16,11 @@ import { ENTRY_COMPARTMENT } from '../src/policy-format.js';
 
 const { keys } = Object;
 
+const readPowers = makeReadPowers({ fs, url });
+
 test('captureFromMap() - should resolve with a CaptureResult', async t => {
   t.plan(5);
 
-  const readPowers = makeReadPowers({ fs, url });
   const moduleLocation = `${new URL(
     'fixtures-0/node_modules/bundle/main.js',
     import.meta.url,
@@ -66,8 +67,90 @@ test('captureFromMap() - should resolve with a CaptureResult', async t => {
   );
 });
 
+test('captureFromMap() - should call _redundantPreloadHook for already-loaded compartments', async t => {
+  t.plan(2);
+  const moduleLocation = `${new URL(
+    'fixtures-0/node_modules/bundle/main.js',
+    import.meta.url,
+  )}`;
+
+  const nodeCompartmentMap = await mapNodeModules(readPowers, moduleLocation);
+
+  /** @type {{ canonicalName: string, entry: string }[]} */
+  const hookCalls = [];
+
+  await captureFromMap(readPowers, nodeCompartmentMap, {
+    _preload: ['bundle-dep'],
+    _redundantPreloadHook: ({ canonicalName, entry }) => {
+      hookCalls.push({ canonicalName, entry });
+    },
+    parserForLanguage: defaultParserForLanguage,
+  });
+
+  t.is(
+    hookCalls.length,
+    1,
+    '_redundantPreloadHook should have been called once',
+  );
+  t.deepEqual(
+    hookCalls[0],
+    { canonicalName: 'bundle-dep', entry: '.' },
+    'hook should have been called with the correct parameters',
+  );
+});
+
+test('captureFromMap() - should only call _redundantPreloadHook for the entry already loaded', async t => {
+  t.plan(3);
+  const moduleLocation = `${new URL(
+    'fixtures-digest/node_modules/app2/index.js',
+    import.meta.url,
+  )}`;
+
+  const nodeCompartmentMap = await mapNodeModules(readPowers, moduleLocation);
+
+  const fjordCompartment = Object.values(nodeCompartmentMap.compartments).find(
+    c => c.name === 'fjord',
+  );
+  if (!fjordCompartment) {
+    t.fail('Expected "fjord" compartment to be present in nodeCompartmentMap');
+    return;
+  }
+
+  /** @type {{ canonicalName: string, entry: string }[]} */
+  const hookCalls = [];
+
+  const { captureCompartmentMap } = await captureFromMap(
+    readPowers,
+    nodeCompartmentMap,
+    {
+      _preload: [
+        'fjord',
+        { compartment: 'fjord', entry: './some-other-entry.js' },
+      ],
+      _redundantPreloadHook: ({ canonicalName, entry }) => {
+        hookCalls.push({ canonicalName, entry });
+      },
+      parserForLanguage: defaultParserForLanguage,
+    },
+  );
+
+  t.true(
+    'fjord' in captureCompartmentMap.compartments,
+    '"fjord" should be retained in captureCompartmentMap',
+  );
+  t.is(
+    hookCalls.length,
+    1,
+    '_redundantPreloadHook should have been called exactly once',
+  );
+  t.deepEqual(
+    hookCalls[0],
+    { canonicalName: 'fjord', entry: '.' },
+    'hook should have fired for the default entry which was already loaded',
+  );
+});
+
 test('captureFromMap() - should preload with canonical name', async t => {
-  const readPowers = makeReadPowers({ fs, url });
   const moduleLocation = `${new URL(
     'fixtures-digest/node_modules/app/index.js',
     import.meta.url,
@@ -83,11 +166,17 @@ test('captureFromMap() - should preload with canonical name', async t => {
     return;
   }
 
+  /** @type {{ canonicalName: string, entry: string }[]} */
+  const hookCalls = [];
+
   const { captureCompartmentMap } = await captureFromMap(
     readPowers,
     nodeCompartmentMap,
     {
       _preload: [fjordCompartment.location],
+      _redundantPreloadHook: ({ canonicalName, entry }) => {
+        hookCalls.push({ canonicalName, entry });
+      },
       parserForLanguage: defaultParserForLanguage,
     },
   );
@@ -96,10 +185,14 @@ test('captureFromMap() - should preload with canonical name', async t => {
     'fjord' in captureCompartmentMap.compartments,
     '"fjord" should be retained in captureCompartmentMap',
   );
+  t.is(
+    hookCalls.length,
+    0,
+    '_redundantPreloadHook should not have been called for a non-redundant preload',
+  );
 });
 
 test('captureFromMap() - should discard unretained CompartmentDescriptors', async t => {
-  const readPowers = makeReadPowers({ fs, url });
   const moduleLocation = `${new URL(
     'fixtures-digest/node_modules/app/index.js',
     import.meta.url,
@@ -141,7 +234,6 @@ test('captureFromMap() - should discard unretained CompartmentDescriptors', asyn
 });
 
 test('captureFromMap() - should preload custom entry', async t => {
-  const readPowers = makeReadPowers({ fs, url });
   const moduleLocation = `${new URL(
     'fixtures-digest/node_modules/app/index.js',
     import.meta.url,
@@ -183,7 +275,6 @@ test('captureFromMap() - should preload custom entry', async t => {
 });
 
 test('captureFromMap() - should round-trip sources based on parsers', async t => {
-  const readPowers = makeReadPowers({ fs, url });
   const moduleLocation = `${new URL(
     'fixtures-0/node_modules/bundle/main.js',
     import.meta.url,