feat(ses,module-source): Dynamic import #2639

kriskowal · 2024-11-20T02:25:02Z

Closes: #291

Description

This change adds support for dynamic import to ses and @endo/module-source, such that import(x) in the scope of a module loaded from source (using ModuleSource in a Compartment module loading hook).

Security Considerations

This change builds on prior work to ensure that dynamic import is facilitated by a hidden lexical name injected by ses. The dynamic import mechanism is isolated to the surrounding compartment and specifiers are resolved on the surrounding module.

Scaling Considerations

This change introduces a static analysis for the presence of a dynamic import in the module source, which it communicates on the module source, even if marshaled through JSON, to ses that it should create an import closure bound to the calling module’s base specifier. This avoids an allocation for most modules.

Documentation Considerations

Only news included. Dynamic import is a language feature that is expected to work in general and documented where JavaScript is documented as a language.

Testing Considerations

This change includes a minimal happy path test that verifies that dynamic import produces a promise that settles on a module namespace object. Note that dynamic import does not box namespaces regardless of the __noNamespaceBox__ compartment option.

Compatibility Considerations

Backward compatible.

Upgrade Considerations

None.

naugtur · 2024-11-21T19:11:12Z

packages/compartment-mapper/test/optional.test.js

@@ -41,7 +42,7 @@ scaffold(
    );
  },
  4,
-  { knownFailure: true },
+  { knownArchiveFailure: true },


(not a polite way to say you should, just a note)
If you want to assert for both behaviors you could pass an assertion to scaffold that uses testCategoryHint to decide what to assert instead of this.

Yeah, this is a sign things are getting weedy. We might refactor knowFailure to be a callback in the future.

boneskull

🎉

boneskull · 2024-11-25T21:36:11Z

packages/compartment-mapper/test/optional.test.js

+  // fails for archives because dynamic import cannot reach modules not
+  // discovered during archival


does dynamic require have the same limitation?

boneskull · 2024-11-25T21:36:48Z

packages/module-source/NEWS.md

+- Supports dynamic `import` within a `ModuleSource` in conjunction with
+  a related change in `ses`.


Suggested change

- Supports dynamic `import` within a `ModuleSource` in conjunction with

a related change in `ses`.

- Supports dynamic `import` (i.e. `await import(specifier)`) within a `ModuleSource` in conjunction with

a related change in `ses`.

boneskull · 2024-11-25T21:38:16Z

packages/ses/NEWS.md

+- Adds support for dynamic `import` in conjunction with an update to
+  `@endo/module-source`.
+
+- Specifying the long discontinued `mathTaming` or `dateTaming` options logs a


Suggested change

- Specifying the long discontinued `mathTaming` or `dateTaming` options logs a

- Specifying the long-discontinued `mathTaming` or `dateTaming` options logs a

boneskull · 2024-11-25T21:39:20Z

packages/ses/src/compartment.js

+    /**
+     * @param {string} fullSpecifier
+     */


What is a fullSpecifier?

Please add description of function

Please add return type of function

Answering in comment, but for you:

A full specifier is a key in the compartment's memo of known modules. When we add support for import attributes (import ansiSys from 'autoexec.bat' with { type: 'bmp' }), the full specifier will be a part of the compartment memo key. Functions like compartment.import take full specifiers. In Node.js and the web, the full specifier happens to be a fully qualified URL, so import.meta.url is the same. In Compartment, the location and full specifier are not necessarily the same. With Compartment Mapper, the full specifier is the specifier you would see in a package.json, like ./src/compartment.js (for internal linkage)orses` (for external linkage).

an import specifier by contrast is a specifier in the context of the surrounding module’s full specifier, like ./module-instance.js is an import specifier in ./src/compartment.js that resolves to a full specifier: ./src/module-instance.js.

you will note that a relative specifier like ./foo.js can has different implications for full specifiers and import specifiers. The ./ prefix implies “internal” for a full specifier and full specifiers cannot have .. path components. The ./ prefix for an import specifier implies that it should be resolved relative to the importer’s full specifier and .. path components should eat the preceding path component of the full specifier.

I don’t have active recall for the terms Node.js chose to distinguish these.

boneskull · 2024-11-25T21:39:54Z

packages/ses/test/import.test.js

+    __options__: true,
+    __noNamespaceBox__: true,


Out of curiosity: wtf are these?

Inadequately documented: https://github.com/endojs/endo/blob/master/packages/ses/NEWS.md#v160-2024-07-30

The legacy signature of Compartment is new Compartment(globals, modules, options) but I observed that globals and modules are both optional, so convinced the stake holders to change this to new Compartment(options), with globals and modules tucked into the options bag. To make that migration without any practical breaking changes, I introduced __options__ to the options so if you use that all the time now, your code is portable between SES and XS.

The legacy signature of compartment.import() returned a Promise<{ namespace: ModuleExportsNamespace }> which has the upside of not running afoul of module namespace objects that happen to export something named then, but has the downside of being inconsistent with dynamic import. The Module Harmony folks have agreed that the thenable modules hazard is a reality we must acknowledge and can’t paper over going forward. This option elects to opt-in to the current standard proposal behavior and behaves consistently with XS.

boneskull · 2024-11-25T21:40:22Z

packages/ses/test/import.test.js

+    __noNamespaceBox__: true,
+    resolveHook: s => s,
+    modules: {
+      '-': {


Does this have a specific meaning, or could it have been foo?

It could have been 'foo', '.', or even ''.

boneskull · 2024-11-25T21:42:09Z

packages/ses/src/module-instance.js

@@ -171,6 +173,13 @@ export const makeModuleInstance = (
    importMetaHook(moduleSpecifier, importMeta);
  }

+  let dynamicImport;


needs /** @type {x} */

boneskull · 2024-11-25T21:47:49Z

packages/ses/test/import.test.js

What if you added a test for CommonJS? Or would that be of low value?

Middling value. We don’t test CJS directly at this layer of abstraction so much as we test the mechanisms for supporting it, which are virtual module sources and compartment.importNow, neither of which go through the dynamic async import path. The exercise of answering your question convinces me that CJS support is orthogonal to this feature.

kriskowal requested a review from naugtur November 20, 2024 02:25

kriskowal force-pushed the kriskowal-dynamic-import-291 branch 2 times, most recently from 6b695d3 to 430a7cf Compare November 20, 2024 22:42

kriskowal requested a review from erights November 21, 2024 01:30

naugtur reviewed Nov 21, 2024

View reviewed changes

boneskull approved these changes Nov 25, 2024

View reviewed changes

kriskowal force-pushed the kriskowal-dynamic-import-291 branch from 430a7cf to a38f4a2 Compare November 26, 2024 01:14

kriskowal added 3 commits November 25, 2024 17:16

feat(module-source): Support for dynamic import

25414d6

feat(ses): Support dynamic import

e56cc04

fix(compartment-mapper): Qualify dynamic import test failure cases

f470696

kriskowal force-pushed the kriskowal-dynamic-import-291 branch from a38f4a2 to f470696 Compare November 26, 2024 01:16

kriskowal enabled auto-merge November 26, 2024 01:21

kriskowal merged commit c6e795e into master Nov 26, 2024
15 checks passed

kriskowal deleted the kriskowal-dynamic-import-291 branch November 26, 2024 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ses,module-source): Dynamic import #2639

feat(ses,module-source): Dynamic import #2639

kriskowal commented Nov 20, 2024

naugtur Nov 21, 2024

kriskowal Nov 26, 2024

boneskull left a comment

boneskull Nov 25, 2024

kriskowal Nov 26, 2024

boneskull Nov 25, 2024

boneskull Nov 25, 2024

boneskull Nov 25, 2024

kriskowal Nov 26, 2024

boneskull Nov 25, 2024

kriskowal Nov 26, 2024

boneskull Nov 25, 2024

kriskowal Nov 26, 2024

boneskull Nov 25, 2024

boneskull Nov 25, 2024

kriskowal Nov 26, 2024

		// fails for archives because dynamic import cannot reach modules not
		// discovered during archival

		- Supports dynamic `import` within a `ModuleSource` in conjunction with
		a related change in `ses`.

	- Specifying the long discontinued `mathTaming` or `dateTaming` options logs a
	- Specifying the long-discontinued `mathTaming` or `dateTaming` options logs a

feat(ses,module-source): Dynamic import #2639

feat(ses,module-source): Dynamic import #2639

Conversation

kriskowal commented Nov 20, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull left a 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

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