Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/compartment-mapper/test/optional.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ const fixtureOptionalDepsCjs = new URL(

scaffold(
'optionalDependencies/esm',
// this test fails because it relies on dynamic import
// fails for archives because dynamic import cannot reach modules not
// discovered during archival
Comment on lines +18 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does dynamic require have the same limitation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

test,
fixtureOptionalDepsEsm,
async (t, { namespace }) => {
Expand All @@ -41,7 +42,7 @@ scaffold(
);
},
4,
{ knownFailure: true },
{ knownArchiveFailure: true },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);

scaffold(
Expand Down
6 changes: 5 additions & 1 deletion packages/compartment-mapper/test/scaffold.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export function scaffold(
addGlobals = {},
policy,
knownFailure = false,
knownArchiveFailure = false,
tags = undefined,
conditions = tags,
strict = false,
Expand All @@ -111,7 +112,10 @@ export function scaffold(
// wrapping each time allows for convenient use of test.only
const wrap = (testFunc, testCategoryHint) => (title, implementation) => {
// mark as known failure if available (but fallback to support test.only)
if (knownFailure) {
if (
knownFailure ||
(knownArchiveFailure && testCategoryHint === 'Archive')
) {
testFunc = testFunc.failing || testFunc;
}
return testFunc(title, async t => {
Expand Down
4 changes: 4 additions & 0 deletions packages/module-source/NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ User-visible changes in `@endo/module-source`:

# Next release

- Supports dynamic `import` within a `ModuleSource` in conjunction with
a related change in `ses`.
Comment on lines +5 to +6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

For example, `await import(specifier)` can now call through to the
surrounding compartment's `importHook` to load and evaluate further modules.
- Provides an XS-specific variant of `@endo/module-source` that adapts the
native `ModuleSource` instead of entraining Babel.

Expand Down
2 changes: 2 additions & 0 deletions packages/module-source/src/babelPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function makeModulePlugins(options) {
reexportMap,
liveExportMap,
importMeta,
dynamicImport,
} = options;

if (sourceType !== 'module') {
Expand Down Expand Up @@ -286,6 +287,7 @@ function makeModulePlugins(options) {
CallExpression(path) {
// import(FOO) -> $h_import(FOO)
if (path.node.callee.type === 'Import') {
dynamicImport.present = true;
path.node.callee = hiddenIdentifier(h.HIDDEN_IMPORT);
}
},
Expand Down
2 changes: 2 additions & 0 deletions packages/module-source/src/module-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export function ModuleSource(source, opts = {}) {
reexportMap,
fixedExportMap,
exportAlls,
needsImport,
needsImportMeta,
} = analyzeModule(source, opts);
this.imports = freeze([...keys(imports)]);
Expand All @@ -97,6 +98,7 @@ export function ModuleSource(source, opts = {}) {
this.__liveExportMap__ = liveExportMap;
this.__reexportMap__ = reexportMap;
this.__fixedExportMap__ = fixedExportMap;
this.__needsImport__ = needsImport;
this.__needsImportMeta__ = needsImportMeta;
freeze(this);
}
Expand Down
3 changes: 3 additions & 0 deletions packages/module-source/src/transform-analyze.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const makeCreateStaticRecord = transformSource =>
hoistedDecls: [],
importSources: Object.create(null),
importDecls: [],
dynamicImport: { present: false },
// enables passing import.meta usage hints up.
importMeta: { present: false },
};
Expand Down Expand Up @@ -103,6 +104,7 @@ const makeCreateStaticRecord = transformSource =>
imports: ${h.HIDDEN_IMPORTS}, \
liveVar: ${h.HIDDEN_LIVE}, \
onceVar: ${h.HIDDEN_ONCE}, \
import: ${h.HIDDEN_IMPORT}, \
importMeta: ${h.HIDDEN_META}, \
}) => (function () { 'use strict'; \
${preamble} \
Expand All @@ -119,6 +121,7 @@ const makeCreateStaticRecord = transformSource =>
liveExportMap: freeze(sourceOptions.liveExportMap),
fixedExportMap: freeze(sourceOptions.fixedExportMap),
reexportMap: freeze(sourceOptions.reexportMap),
needsImport: sourceOptions.dynamicImport.present,
needsImportMeta: sourceOptions.importMeta.present,
functorSource,
});
Expand Down
6 changes: 5 additions & 1 deletion packages/ses/NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ User-visible changes in `ses`:

# Next version

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

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

# v1.10.0 (2024-11-13)

Expand Down
42 changes: 40 additions & 2 deletions packages/ses/src/compartment.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ import {
setGlobalObjectMutableProperties,
setGlobalObjectEvaluators,
} from './global-object.js';
import { assert, assertEqual } from './error/assert.js';
import { assert, assertEqual, q } from './error/assert.js';
import { sharedGlobalPropertyNames } from './permits.js';
import { load, loadNow } from './module-load.js';
import { link } from './module-link.js';
import { getDeferredExports } from './module-proxy.js';
import { compartmentEvaluate } from './compartment-evaluate.js';
import { makeSafeEvaluator } from './make-safe-evaluator.js';

/** @import {ModuleDescriptor} from '../types.js' */
/** @import {ModuleDescriptor, ModuleExportsNamespace} from '../types.js' */

// moduleAliases associates every public module exports namespace with its
// corresponding compartment and specifier so they can be used to link modules
Expand Down Expand Up @@ -297,6 +297,43 @@ export const makeCompartmentConstructor = (

assign(globalObject, endowments);

/**
* In support dynamic import in a module source loaded by this compartment,
* like `await import(importSpecifier)`, induces this compartment to import
* a module, returning a promise for the resulting module exports
* namespace.
* Unlike `compartment.import`, never creates a box object for the
* namespace as that behavior is deprecated and inconsistent with the
* standard behavior of dynamic import.
* Obliges the caller to resolve import specifiers to their corresponding
* full specifier.
* That is, every module must have its own dynamic import function that
* closes over the surrounding module's full module specifier and calls
* through to this function.
* @param {string} fullSpecifier - A full specifier is a key in the
* compartment's module memo.
* The method `compartment.import` accepts a full specifier, but dynamic
* import accepts an import specifier and resolves it to a full specifier
* relative to the calling module's full specifier.
* @returns {Promise<ModuleExportsNamespace>}
*/
const compartmentImport = async fullSpecifier => {
if (typeof resolveHook !== 'function') {
throw new TypeError(
`Compartment does not support dynamic import: no configured resolveHook for compartment ${q(name)}`,
);
}
await load(privateFields, moduleAliases, this, fullSpecifier);
const { execute, exportsProxy } = link(
privateFields,
moduleAliases,
this,
fullSpecifier,
);
execute();
return exportsProxy;
};

weakmapSet(privateFields, this, {
name: `${name}`,
globalTransforms,
Expand All @@ -314,6 +351,7 @@ export const makeCompartmentConstructor = (
instances,
parentCompartment,
noNamespaceBox,
compartmentImport,
});
}

Expand Down
15 changes: 14 additions & 1 deletion packages/ses/src/module-instance.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/** @import {ModuleExportsNamespace} from '../types.js' */

import { assert } from './error/assert.js';
import { getDeferredExports } from './module-proxy.js';
import {
Expand Down Expand Up @@ -131,13 +133,15 @@ export const makeModuleInstance = (
__fixedExportMap__: fixedExportMap = {},
__liveExportMap__: liveExportMap = {},
__reexportMap__: reexportMap = {},
__needsImport__: needsImport = false,
__needsImportMeta__: needsImportMeta = false,
__syncModuleFunctor__,
} = moduleSource;

const compartmentFields = weakmapGet(privateFields, compartment);

const { __shimTransforms__, importMetaHook } = compartmentFields;
const { __shimTransforms__, resolveHook, importMetaHook, compartmentImport } =
compartmentFields;

const { exportsProxy, exportsTarget, activate } = getDeferredExports(
compartment,
Expand Down Expand Up @@ -171,6 +175,14 @@ export const makeModuleInstance = (
importMetaHook(moduleSpecifier, importMeta);
}

/** @type {(fullSpecifier: string) => Promise<ModuleExportsNamespace>} */
let dynamicImport;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs /** @type {x} */

if (needsImport) {
/** @param {string} importSpecifier */
dynamicImport = async importSpecifier =>
compartmentImport(resolveHook(importSpecifier, moduleSpecifier));
}

// {_localName_: [{get, set, notify}]} used to merge all the export updaters.
const localGetNotify = create(null);

Expand Down Expand Up @@ -462,6 +474,7 @@ export const makeModuleInstance = (
imports: freeze(imports),
onceVar: freeze(onceVar),
liveVar: freeze(liveVar),
import: dynamicImport,
importMeta,
}),
);
Expand Down
19 changes: 19 additions & 0 deletions packages/ses/test/import.test.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
/* eslint max-lines: 0 */

import test from 'ava';
import { ModuleSource } from '@endo/module-source';
import '../index.js';
import { resolveNode, makeNodeImporter } from './_node.js';
import { makeImporter, makeStaticRetriever } from './_import-commons.js';
Expand Down Expand Up @@ -600,3 +601,21 @@ test('importMetaHook and meta from record', async t => {
const { default: metaurl } = await compartment.import('./index.js');
t.is(metaurl, 'https://example.com/index.js?foo');
});

test('dynamic import from source', async t => {
const c = new Compartment({
__options__: true,
__noNamespaceBox__: true,
Comment on lines +607 to +608
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: wtf are these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

resolveHook: s => s,
modules: {
'-': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

source: new ModuleSource(`
export const dynamic = import('-');
`),
},
},
});
const namespace = await c.import('-');
const namespace2 = await namespace.dynamic;
t.is(namespace, namespace2);
});
Loading