exitModuleImportHook alongside synchronous exit module linking #1506

naugtur · 2023-03-03T14:05:36Z

Abandoned in favor of #1507

This is a PoC exitModuleImportHook.

It works, but not necessarily the way we want it to work.

Notes:

It seems to make sense for the exitModuleImportHook to return a ThirdPartyStaticModuleInterface
I am exploring replacing exitModules with the exitModuleImportHook entirely, which removes a lot of complications from linking, changes module attenuation slightly (might be for the better, not sure yet) but it changes how modules from external assemblies or compartments could get included. I don't see a lot of use for that though.
I don't yet see a way to properly attenuate on top of exitModuleImportHook - I just discovered that attenuation would only run once per builtin and then it gets cached by the specifier, so attenuation would have to return a unique alias record at least. I'm not sure if that's enough.

packages/compartment-mapper/src/import-archive.js

naugtur · 2023-03-03T14:23:20Z

packages/compartment-mapper/src/import-archive.js

@@ -264,6 +285,7 @@ export const parseArchive = async (
      archiveLocation,
      computeSha512,
      computeSourceLocation,
+      exitModuleImportHook,


I don't enjoy passing the hook to the archive making part of the archive flow.
It should not be necessary before runtime.

Solutions:

A boolean configuration value saying to "expectExitModuleImportHook" would be enough to omit the archive-time errors where exit import hook would kick in.
pros: no unnecessary importing and hook mechanics before runtime
cons: no early errors if exitModuleImportHook doesn't provide a module, another config item

exitModuleImportHook accepts options, including archiveOnly and if that's truthy, it can avoid importing actual modules while still returning undefined where it originally would (eg. if checking against a list of known builtins it wishes to support)
pros: no additional option, no need to import actual modules during archiving
cons: more complexity in exitModuleImportHook

I would rename this parameter to isExitModuleImportAllowed, since the logic for it may be distinct from the logic of exitModuleImportHook, and it only needs to return truthy or falsy (default: () => false). Using myExitModuleImportHook for the parameter is a degenerate case that works, but isn't ideal (it does too much work, as you point out above).

naugtur · 2023-03-03T14:25:45Z

packages/compartment-mapper/src/types.js

@@ -239,6 +245,7 @@ export {};
 * @property {Array<Transform>} [transforms]
 * @property {Array<Transform>} [__shimTransforms__]
 * @property {Record<string, object>} [modules]
+ * @property {ExitModuleImportHook} [exitModuleImportHook]


I'm wondering if

* @property {Record<string, object> | ExitModuleImportHook} [modules]

would be frowned upon.

I don't see a reasonable usecase for passing both. If passing a function, wrapping it in an object lookup of sorts is pretty much a necessity anyway.

I'd prefer not to mix types in this way in the API (policy description type mixing as you've done in the past is a different matter, for reasons of needing to be concise). I'd rather have just an assertion around !modules || !exitModuleImportHook.

I'm willing to remove the modules option entirely if you mark it as a breaking change, and update any uses within endo that depend on it to use the exitModuleImportHook instead.

I see you're assuming this is more ready than it really is.
I experimented with removing the modules option entirely and it does improve a bunch of things, but also has some interesting consequences. I'll talk about it in the meeting.

naugtur · 2023-03-03T14:26:43Z

packages/compartment-mapper/test/scaffold.js

@@ -78,6 +78,7 @@ export function scaffold(
    tags = undefined,
    searchSuffixes = undefined,
    commonDependencies = undefined,
+    additionalOptions = {},


Can't extend scaffold with new minor bits of logic forever...

michaelfig · 2023-03-07T15:43:18Z

packages/compartment-mapper/src/import-archive.js

@@ -97,6 +100,16 @@ const makeArchiveImportHookMaker = (
      // per-module:
      const module = modules[moduleSpecifier];
      if (module === undefined) {
+        if (exitModuleImportHook) {
+          console.error('#################x');


Leftover debug message.

michaelfig · 2023-03-07T15:44:56Z

packages/compartment-mapper/src/import-hook.js

@@ -156,6 +158,21 @@ export const makeImportHookMaker = (
          // Archived compartments are not executed.
          return freeze({ imports: [], exports: [], execute() {} });
        }
+        if (exitModuleImportHook) {
+          console.error('#################i');


Here again.

yes, it's still early in the implementation and these are useful for showing when and what happens. I'll need them for the meeting.

michaelfig · 2023-03-07T15:47:24Z

packages/compartment-mapper/src/types.js

@@ -239,6 +245,7 @@ export {};
 * @property {Array<Transform>} [transforms]
 * @property {Array<Transform>} [__shimTransforms__]
 * @property {Record<string, object>} [modules]
+ * @property {ExitModuleImportHook} [exitModuleImportHook]


I'm willing to remove the modules option entirely if you mark it as a breaking change, and update any uses within endo that depend on it to use the exitModuleImportHook instead.

naugtur · 2023-03-08T09:12:39Z

packages/compartment-mapper/src/import-hook.js

+            // note it's not being marked as exit in sources
+            // it could get marked and the second pass, when the archive is being executed, would have the data
+            // to enforce which exits can be dynamically imported
+            return record;


Introducing attenuation by policy in here is going to be difficult, because each time a builtin is imported it'd have to get a new instance for different attenuations and here it'll get memoized.

naugtur commented Mar 3, 2023

View reviewed changes

packages/compartment-mapper/src/import-archive.js Outdated Show resolved Hide resolved

naugtur commented Mar 3, 2023

View reviewed changes

naugtur force-pushed the naugtur-exit-import-hook branch 2 times, most recently from 23dd12b to 31ab2f9 Compare March 3, 2023 14:28

squashme poc

5cd0309

naugtur force-pushed the naugtur-exit-import-hook branch from 31ab2f9 to 5cd0309 Compare March 3, 2023 14:43

naugtur requested review from michaelfig and kumavis March 6, 2023 09:47

hook returns a record

70b2971

michaelfig reviewed Mar 7, 2023

View reviewed changes

naugtur commented Mar 8, 2023

View reviewed changes

naugtur changed the title ~~exitModuleImportHook~~ exitModuleImportHook alongside synchronous exit module linking Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exitModuleImportHook alongside synchronous exit module linking #1506

exitModuleImportHook alongside synchronous exit module linking #1506

naugtur commented Mar 3, 2023 •

edited

Loading

naugtur Mar 3, 2023

michaelfig Mar 6, 2023

naugtur Mar 3, 2023 •

edited

Loading

michaelfig Mar 6, 2023

michaelfig Mar 7, 2023

naugtur Mar 8, 2023

naugtur Mar 3, 2023

michaelfig Mar 7, 2023

michaelfig Mar 7, 2023

naugtur Mar 8, 2023

michaelfig Mar 7, 2023

naugtur Mar 8, 2023

exitModuleImportHook alongside synchronous exit module linking #1506

Are you sure you want to change the base?

exitModuleImportHook alongside synchronous exit module linking #1506

Conversation

naugtur commented Mar 3, 2023 • edited Loading

Abandoned in favor of #1507

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naugtur Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naugtur commented Mar 3, 2023 •

edited

Loading

naugtur Mar 3, 2023 •

edited

Loading