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

Divide into low-level chapters #71

Merged
merged 19 commits into from
Jul 19, 2022
Merged

Divide into low-level chapters #71

merged 19 commits into from
Jul 19, 2022

Conversation

kriskowal
Copy link
Member

The last month of conversations with champions of this and other module proposals, notably @caridy, @littledan, and @nicolo-ribaudo, has convinced me that it is possible to create Compartment from constituent lower-level parts, Module, ModuleSource, and Evaluators. To that end, @caridy and I’ve sketched a possible direction for the compartments proposal that would clarify the intersection semantics with module blocks, deferred execution, and import reflection; and motivating cases for each layer of the compartments proposal so that we can converse honestly about the coupling of each of these parts, for which I believe there is ample support to motivate every layer.

2.md Outdated
Comment on lines 89 to 117
### WASM

On a host that does not provide support for WebAssembly, a virtual module
source would suffice.

```js
class WasmModuleSource {
constructor(buffer) {
const module = new WebAssembly.Module(buffer);
this.#imports = WebAssembly.Module.imports(module);
this.#exports = WebAssembly.Module.exports(module);
this.bindings = [
...this.#imports.map(({ module, name }) =>
({ import: name, from: module })),
...this.#exports.map(({ name }) =>
({ export: name })),
];
};
async execute(namespace) {
const importObject = {};
for (const { module, name, kind } of this.#imports) {
importObject[name] = namespace[name];
}
const instance = await WebAssembly.instantiate(module, importObject);
for (const { name } of this.#exports) {
namespace[name] = instance[name];
}
};
}
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @guybedford I think this might be correct but defer to your expertise. This of course does not preclude the possibility of WebAssembly.Module becoming a better second-class option in hosts that support WASM directly. I’ve written text to that effect elsewhere.

@kriskowal kriskowal force-pushed the chapters branch 2 times, most recently from 2cf5f3f to b0f19fc Compare July 16, 2022 00:26
0.md Outdated Show resolved Hide resolved
0.md Outdated Show resolved Hide resolved
0.md Outdated Show resolved Hide resolved
1.md Outdated Show resolved Hide resolved
1.md Outdated Show resolved Hide resolved
1.md Outdated
Comment on lines 188 to 190
With the design as written, `needsImport` would only be `false` for modules
that make no use of static `import` or `export` `from` clauses and also never
use the syntactic form for dynamic `import`.
Copy link
Member

Choose a reason for hiding this comment

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

I thought needsImport means needs dynamic import. Static imports and exports are resolved without any call to the context.import().

Each module will need to have its own context.import function (in order to distinguish the import referrer) so having this property also allows to save more memory.

Copy link
Member Author

Choose a reason for hiding this comment

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

With @caridy’s design, we use [[Context]].[[Evaluators]].[[ImportHook]] for both static and dynamic import, and the ImportHook receives the importMeta, so a single importHook can serve a large module graph.

The virtual module source chapter does end up reading needsImport, but that is indeed a case where the import function needs to be bound to the referrer and only necessary for dynamic import. All the static imports on the namespace objects before calling execute / initialize.

2.md Outdated Show resolved Hide resolved
2.md Outdated Show resolved Hide resolved
2.md Outdated Show resolved Hide resolved
2.md Outdated Show resolved Hide resolved
2.md Outdated Show resolved Hide resolved
1.md Outdated Show resolved Hide resolved
2.md Outdated Show resolved Hide resolved
0.md Outdated Show resolved Hide resolved
0.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@erights erights left a comment

Choose a reason for hiding this comment

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

Review so far. Not yet done.

README.md Outdated Show resolved Hide resolved
0.md Outdated Show resolved Hide resolved
0.md Outdated Show resolved Hide resolved
0.md Outdated Show resolved Hide resolved
0.md Outdated
Note 2: Multiple `Module` instances can be constructed from a single `ModuleSource`,
producing one exports namespaces for each imported `Module` instance.

Note 3: The internal record of a `ModuleSource` instance is immutable and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good that you made it clear that it is not the "same" object that is shared between agents.

0.md Outdated
***Module Namespace Object***

Dynamic import induces calls to `importHook` for each unsatisfied dependency of
each module instance in separate events, before any dependency advances to the
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is an "event"? Is this a 262 term?

Copy link
Member Author

Choose a reason for hiding this comment

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

I need guidance here. 262 seems to frame all things in terms of the LIFO or FIFO hand-off among execution contexts, using the same mechanism for both the stack and job queue. I’ll have to look at promise jobs to see how they handle this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know the answer, but that is the best first place to look.

0.md Outdated
Dynamic import within the evaluation of a `Module` also invokes the
`importHook`.

`Module` instances memoize the result of their `importHook` keyed on the given
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am suddenly confused about the directionality of the passing of importMeta. We should discuss.

Copy link
Member Author

Choose a reason for hiding this comment

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

Module instances are constructed with an importMeta. Module instances pass their importMeta to the importHook, which may be shared by multiple Module instances, to indicate the referrer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am now even more confused ;) . We should definitely discuss. (But not now, sorry)

0.md Outdated Show resolved Hide resolved
0.md Outdated
Comment on lines 95 to 96
Any dynamic import function is suitable for initializing, linking, and
evaluating a module instance and all of its transitive dependencies.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that the immediate importHook returns a (promise for a) module instance, and that module instance presumably has its own importHook, how does the transitive use of the original importHook arise at this level? I suspect it does not and this "transitive" explanation should be omitted at this level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Import hooks do produce shallow Module instances and import hooks may return module instances in any state of their lifecycle.

This section describes dynamic import, which guarantees that the given module instance is advanced to its terminal state. I’m not sure whether it’s possible to advance a module to its terminal state without advancing its transitive dependencies to their terminal state too.

Copy link
Member

Choose a reason for hiding this comment

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

how does the transitive use of the original importHook arise at this level?

I believe (and I implemented it as) it does not "transitive". When resolving modules, it will call the parent module's importHook, and every resolved module T will have its own importHook to resolve T's dependencies, so the importHook only affects 1 level.

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, each Module instance uses its own importHook. Dynamic import asynchronously advances the target Module instance to its terminal state. That implies advancing its transitive dependencies, either all to their terminal success state, or any one of them to its terminal failure state.

0.md Outdated Show resolved Hide resolved
2.md Outdated
Comment on lines 111 to 120
async execute(namespace) {
const importObject = {};
for (const { module, name, kind } of this.#imports) {
importObject[name] = namespace[name];
}
const instance = await WebAssembly.instantiate(module, importObject);
for (const { name } of this.#exports) {
namespace[name] = instance[name];
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Is namespace both for receiving the imports, and for exporting them? What if I want to export something with the same name as an imported value?

Copy link
Member

Choose a reason for hiding this comment

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

Oh that's probably why we have as for imports.

Copy link
Member Author

Choose a reason for hiding this comment

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

Avoiding such collisions is the purview of bindings, where as clauses can resolve conflicts. The namespace object reflects the bound subset of the names an ESM will have in its global contour.

execute?: (namespace: ModuleImportsNamespace, {
import?: (importSpecifier: string) => Promise<ModuleExportsNamespace>,
importMeta?: Object,
globalThis?: Object,
Copy link
Member

Choose a reason for hiding this comment

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

this property should never be missing. and what about the previous idea? that namespace and globalThis merged into one object as the "import lexical scope" + "global scope"

Copy link
Member Author

Choose a reason for hiding this comment

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

@erights felt that reifying the whole module environment record, including its fall-through to the global environment record, coupled too many concerns. I’ll add this to the design questions.

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’s probably worth noting that reifying the module environment record in its entirety would put us in a better position to implement the “global lexicals” feature using Evaluators, to implement Compartments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be interested in seeing this explored. Frankly, I often forget that we also need to support global lexicals.

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is that Moddable’s XS (cc @patrick-soquet @phoddie) reifies the module environment record and I assumed that meant that it reflected the global lexicals and global object of their Compartment.

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 occurs to me that both strategies are viable. We could also surface globalLexicals as yet another object in the execute arguments bag.

Copy link
Member

Choose a reason for hiding this comment

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

What are the global lexicals?

Copy link
Member

Choose a reason for hiding this comment

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

Like const/let at the top level of Scripts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe let and const at the top level of Scripts are called the “global contour”. There is a separate potential extension for evaluators that would make them more suitable for REPL, where the global contour can be made to persist between Script evaluations. We have not yet figured out whether that’s the same or different than what we’re calling the global lexicals, but it is probably different.

At Agoric, we briefly needed a way to meter a JavaScript program in user code. We eventually found a better way, by subshelling an XS process with an “instructions to process” budget. When we implemented metering in user code, we performed a source transformation that injected meter decrements at junctions. To deny guest code the ability to fiddle with their meter, we censored the name of the meter from source code and needed a place to reveal the variable to object code. The global object was not eligible because any name can be reached by computed key. So we introduced a new “global lexicals” feature to compartments that allowed us to introduce the name just above both the top level lexical scope of modules and scripts. There might be other ways to do this or the case might not be sufficiently motivated, but I’m satisfied that there is a way to do this in the future regardless of whether the feature is revealed through a module environment record or a separate argument in the bag.

@erights erights self-requested a review July 18, 2022 02:50
Copy link
Collaborator

@erights erights left a comment

Choose a reason for hiding this comment

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

Just a bit more progress.

const source = new ModuleSource(``);
const instance = new Module(source, {
importHook,
importMeta: import.meta,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, now I'm even more confused about import.meta. Here, the calling code is passing its own import.meta as the value of the importMeta option. Does this make sense?

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense when you want to share the importMeta with the child module (e.g. child module uses new URL(..., import.meta.url))

Copy link
Member Author

Choose a reason for hiding this comment

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

Passing import.meta is sufficient in cases where the host and guest module share the same referrer and the import.meta directly states the referrer, as in import.meta.url, which is the scope of this example.

It is also possible to arrange for an importHook for which passing import.meta directly from host to guest would not be sufficient, as would be the case if importMeta is used as a key in a WeakMap to address the corresponding referrer, since import.meta is not object identical to the importMeta passed to the Module constructor or importHook. For these cases, more ceremony is necessary but the motivating cases are all supported.

importer, then a new syntax can be provided to only get the `ModuleSource`:

```ts
import source from 'module.js' static source syntax;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: If we have the second, giving us the ModuleSource, does that subsume the need for the first?

If so, and given the static module {} syntax above, could this one be

import source from static 'module.js';

Isn't this exactly the same issue as module reflection? However it is spelled, doesn't the ability to import the ModuleSource subsume all of 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.

Yes, importing module sources overlaps with import reflection and importing module instances overlaps with dynamic import. There are many ways that Module Harmony can unfold that cover all the motivating cases, but I’m in favor of separate module and static module syntaxes for both module blocks and import (by whatever spelling) , to cover both deferred execution and module source reflection.

import module from 'module';
module instancof Module // true, maybe pre-loaded, must not be executed

import static module from 'module';
// module is a module source, albeit `ModuleSource`, `WebAssembly.Module`, or a virtual module source
// maybe virtual module sources are instances of `ModuleSource` at this juncture

(module {}) instanceof Module
(module {}).source instanceof ModuleSource
(static module {}) instanceof ModuleSource

Copy link
Collaborator

Choose a reason for hiding this comment

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

@erights having only the static version of the syntax would be fine IMO, but having access to the "dynamic" version (which is basically static + current import.meta and default importHook) is also very useful and intuitive IMO.


```ts
const importHook = async (specifier, importMeta) => {
const url = importMeta.resolve(specifier);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The import.meta object is ambient and reachable by syntax. I am concerned about how much power it would convey if it has its own resolve function. Would this function cause I/O?

Copy link
Member

Choose a reason for hiding this comment

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

I believe import.meta.resolve is already going to ship in both browsers and node - in browsers I believe it does not do i/o, because it's using the URL cache, and in node I believe it does do i/o, since it checks the disk.

Copy link
Member

Choose a reason for hiding this comment

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

We can't limit what host put on the importMeta, why is that a question? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@erights This is a reductive example. The importMeta can be a key in a side table to the underlying referrer, fully hiding resolution. Also, importMeta will not be object identical to import.meta and not shared by multiple modules. It’ll be copied onto import.meta if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@erights there is a difference between the importMeta provided to the instance of Module, and the import.meta accessible from the evaluation of the source associated to the instance of Module. This is well described in the spec today. In essence, the developer is responsible for providing importMeta object that can be passed to any subsequence invocation of the associated importHook, but when the source access import.meta, a new object is created, and it is populated by the importMeta properties. This means you can easily virtualize what ended up being accessible from within the source via import.meta syntax.

0-module-and-module-source.md Outdated Show resolved Hide resolved
0-module-and-module-source.md Outdated Show resolved Hide resolved
constructor(
source: ModuleSource,
options: {
importHook?: ImportHook,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kriskowal the reason why I went with 2nd and 3rd argument instead of an option bags here is that when importHook is omitted, what is importMeta for then? it seems that they are bound, and if one if present, the other must be present, which is easier to represent via arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is of course still an open question, but we’ll need an options bag regardless for import assertions per #37 and it’s likely that there are reasonable defaults for both of these.

Copy link
Member

Choose a reason for hiding this comment

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

What if I inherit importHook from Evaluator but I want to provide a special importMeta?


- `{ import: string, as: string, from: string }`

For example, `import { a as x } from 'a.js'` would produce
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having a hard time understanding why is x relevant here, since x is internal to the module source. Basically, in order for a tool to replace this import statement, they will need an AST and make transformations, while this is just meta that might help with this transformation but we should probably stay away from exposing internals of the source.


## Design Questions

Do we also need to reflect `isAsync`?
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is again, another internal meta data of the source that should probably not be exposed.

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, I leave this open because revealing it might be necessary for @Jack-Works use case.

Copy link
Member

Choose a reason for hiding this comment

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

This is currently required to be known before execution in the spec algorithm of Execute() method of a Cyclic Module Record.

I'm currently implementing this proposal following the Cyclic Module Record's algorithm. If we have a new spec for it that does not need to know this information, it is not necessary to me.

But I don't agree it's "internal" metadata. This reflects if the module is an Async module or not.

Extend [ModuleSource][0], such that instances have the following properties:

- `bindings`, an `Array` of `Binding`s.
- `needsImportMeta`, a `boolean` indicating that the module contains
Copy link
Collaborator

@caridy caridy Jul 18, 2022

Choose a reason for hiding this comment

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

I don't think this is meta that we should expose it.

Copy link
Member

Choose a reason for hiding this comment

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

agreed - adding or removing use of import.meta should never become observable, and thus a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Presumably needsImportMeta of a module source is the product of observing the most recent source. This is intended to communicate to importHook whether it needs to construct a heavy import.meta (including a resolve closure) or not, and is meaningful to the Node.js community.

Copy link
Member

Choose a reason for hiding this comment

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

That's not an optimization that should be baked into the spec, imo.

Copy link
Member Author

Choose a reason for hiding this comment

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

The optimization necessarily occurs in a user-defined importHook, but the hook needs a way to recognize whether the optimization is possible.

Copy link
Member Author

@kriskowal kriskowal Jul 19, 2022

Choose a reason for hiding this comment

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

To be clear, I do not particularly care about this detail, but I do care about this API being a viable alternative to Node.js’s loader (please forgive hearsay), which has this optimization because it makes a considerable difference to cold start times in a module graph with 1,000 modules where 5 of them use import.meta.resolve and the others don’t mention import.meta at all. Without this hint, they need a considerably number of resolve closures that will never be observed.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason they couldn't be getters on import.meta that lazily create those closures?

Copy link
Member

Choose a reason for hiding this comment

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

getter is a function too. They also have costs to create.

Copy link
Member Author

Choose a reason for hiding this comment

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

First, you are right that there are cases where a ModuleSource will be available but the source text will not. That becomes possible with either a harmonious import reflection or deferred execution proposal, where a CSP-vetted ModuleSource would be exposed but not necessarily its source text. In my opinion, that strengthens the case for exposing static analysis, but I read your disagreement.

Currently, hosts can only provide objects with a null prototype. We could allow new Module() to receive an arbitrary value to present as import.meta. If that were the case, such an object could have a host-defined prototype with accessors, provided that any of the computed properties of one import.meta could be inferred from other properties of import.meta. To wit, an import.meta.resolve that closes over import.meta.url, import.meta.path, or some other state accessible to the implementation of the Meta class (pun intended).

I read in your preference not to expose static analysis as flowing from a respectable abundance of caution. I have no objection to leaving this question unanswered until some time has been allowed to uncover a genuine hazard that we currently avoid by not revealing the static analysis of successfully-linked CSP-vetted modules, and in parallel giving some time to surface objections to this alternative approach of allowing arbitrary import.meta values.

Copy link

@Jamesernator Jamesernator Jul 19, 2022

Choose a reason for hiding this comment

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

That's not an optimization that should be baked into the spec, imo.

So to be clear, the way import.meta works already allows hosts to lazily create import.meta when it's accessed. This proposal is even more conservative than that in that rather than waiting until access-time, it only tells when the whole module is to be executed.

Specifically this behaviour using Node's experimental vm modules is fully spec compliant today:

#!/usr/bin/env -S node --experimental-vm-modules
import vm from "node:vm";

const module = new vm.SourceTextModule(`
   console.log(1);
   void import.meta; // access import.meta
   console.log(3);
`, {
    initializeImportMeta(meta) {
        console.log(2);
    },
});

and prints 1,2,3.

This proposal is more conservative in that you can't observe when import.meta is first accessed, just when the module is executed and whether or not import.meta is ever accessed (not when it's actually accessed).

`Module` constructors, like `Function` constructors, are bound to a realm
and evaluate modules in their particular realm.

## Examples
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kriskowal I think we are missing the section about Module.get(namespace) from the original gist. Without this capability, there is no way to interconnect a manually created module subgraph with the host-created graph, so it is very limiting.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, I’m assuming that the deferred execution proposal or import reflection accommodate this case and intend to circle back for Module.get if necessary or helpful.

Copy link
Member

Choose a reason for hiding this comment

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

What is Module.get(namespace)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Module.get(ModuleNamespace) => Module would reveal the Module underlying a given module exports namespace object, such that it could be returned by an importHook.

## Interface

```ts
type VirtualModuleSource = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Side Note: I strongly believe that this is not the best place, and instead what you want is really a non-transferable (or non-serializable) structure, which ModuleSource is not. I believe a far better choice would be to have a VirtualModule instead, which is going to be far lower level control mechanism, but a lot more powerful and simpler. It might be that you have a combination of VirtualModuleSource + VirtualModule to match the existing ModuleSource + Module, where one is meta and it is serializable, and the other one is not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the case of the VirtualModule, all we need is a way to allow setting values of a list of predefined slots (described by VirtualModuleSource by exported name bindings), which is a well known mechanism used by modules today where the value must be set into the environment record associated to a module.

Again, we go from spec'ing a "parameterized artifact" to "parameterized low-level APIs" so developers can create the artifact themself.

```ts
interface Evaluators {
constructor({
globalThis?: Object,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be problematic. In the past, implementers agreed to allow the creation of global object as ordinary object (which is the case of ShadowRealm today), but providing a global object from user-land, I don't think that will be possible. What if that object is a proxy? There are certain qualities of a global object (specially if it is associated to a module instance) that implementers will like to preserve, and letting them creating such object (ordinary or not) seems important. This is not a deal-breaker, but will require some adjustment of this API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I figure we should ask for this and change it only if there are strong objections, since this is much easier for users to understand than the alternative (which would need separate argument patterns for shared globals and fresh globals).

Copy link
Collaborator

@erights erights left a comment

Choose a reason for hiding this comment

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

In the interests of getting this merged before tc39 plenary, I'm ready to approve but for one change I'm requesting.

As I just discussed with @phoddie and @kriskowal , the importHook should be given a referrer string, and not be given an importMeta or anything corresponding to the import.meta object.

With that changed, the importMeta argument of the Module constructor can then be the value of import.meta within the constructed module. No property copying needed.


This proposal is a [SES Proposal][ses-proposal] milestone.

## Motivation
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused here. What is it that you can't do in user-land with 0, 1, 2 and 3 from above that this (4) will give you?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Higher-level API that would allow for terser orchestration in bundle runtimes, which are sensitive about their weight.
  2. Fewer reified objects. Embedded systems will likely implement Compartment natively regardless of whether compartments can be implemented in user code to avoid unnecessary reification of intermediate objects. Our friends at Moddable are not quite ready to delete layer 4 from the proposal.

@caridy
Copy link
Collaborator

caridy commented Jul 18, 2022

In the interests of getting this merged before tc39 plenary, I'm ready to approve but for one change I'm requesting.

As I just discussed with @phoddie and @kriskowal , the importHook should be given a referrer string, and not be given an importMeta or anything corresponding to the import.meta object.

With that changed, the importMeta argument of the Module constructor can then be the value of import.meta within the constructed module. No property copying needed.

Let's not rush this @erights, I think it is fine to postpone it so we can talk more about this. I disagree with introducing the concept of referral in user-land for various reasons, that's why we landed on importMeta, which is different from import.meta btw, I mentioned in one of my comments.

@kriskowal
Copy link
Member Author

kriskowal commented Jul 18, 2022

In the interests of getting this merged before tc39 plenary, I'm ready to approve but for one change I'm requesting.
As I just discussed with @phoddie and @kriskowal , the importHook should be given a referrer string, and not be given an importMeta or anything corresponding to the import.meta object.
With that changed, the importMeta argument of the Module constructor can then be the value of import.meta within the constructed module. No property copying needed.

Let's not rush this @erights, I think it is fine to postpone it so we can talk more about this. I disagree with introducing the concept of referral in user-land for various reasons, that's why we landed on importMeta, which is different from import.meta btw, I mentioned in one of my comments.

The reason I’m convinced that we need to bring referrer back is that it should not be possible for the referrer to change between calls to importHook, as it can as a property of importMeta. It is a net reduction in confusion if importMeta (the Module constructor argument), importMeta (the importHook argument), and import.meta (as seen by the evaluated module) are identical.

@caridy
Copy link
Collaborator

caridy commented Jul 18, 2022

In the interests of getting this merged before tc39 plenary, I'm ready to approve but for one change I'm requesting.
As I just discussed with @phoddie and @kriskowal , the importHook should be given a referrer string, and not be given an importMeta or anything corresponding to the import.meta object.
With that changed, the importMeta argument of the Module constructor can then be the value of import.meta within the constructed module. No property copying needed.

Let's not rush this @erights, I think it is fine to postpone it so we can talk more about this. I disagree with introducing the concept of referral in user-land for various reasons, that's why we landed on importMeta, which is different from import.meta btw, I mentioned in one of my comments.

The reason I’m convinced that we need to bring referrer back is that it should not be possible for the referrer to change between calls to importHook, as it can as a property of importMeta. It is a net reduction in confusion if importMeta (the Module constructor argument), importMeta (the importHook argument), and import.meta (as seen by the evaluated module) are identical.

I don't think that is fatal, in fact, even if the referral is fixed, the fact that the user can implement whatever logic they want into the associated importHook is sufficient for them to either ignore the referral, or change the behavior associated to it from one invocation to another, that's precisely why we cache the invocation of the hook, so such change in behavior or (change in value) are NOT observable from outside, because they just resolve to a module and you have no way to prove that the module resolved is wrong compared to other modules that you have imported.

@kriskowal
Copy link
Member Author

Rather than answer the questions brought up in comments above, I’ve added these questions to the proposal so that we can hopefully have this PR landed in a consistent state in anticipation of plenary.

@Jack-Works
Copy link
Member

With that changed, the importMeta argument of the Module constructor can then be the value of import.meta within the constructed module. No property copying needed.

I don't agree. If we're not coping properties, the user might provide an exotic importMeta object which maybe is not wanted.

Copy link
Collaborator

@caridy caridy left a comment

Choose a reason for hiding this comment

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

I'm fine merging this as it is, and we can iterate over it.

@caridy
Copy link
Collaborator

caridy commented Jul 19, 2022

With that changed, the importMeta argument of the Module constructor can then be the value of import.meta within the constructed module. No property copying needed.

I don't agree. If we're not coping properties, the user might provide an exotic importMeta object which maybe is not wanted.

I agree with @Jack-Works here, and in fact, there is already a well defined process in 262 to build the import.meta object.

instances of `ModuleSource` between any JavaScript hosts, or even
[`WebAssembly.Module`][web-assembly-module] when a host-defined extension is
present, but it would not be possible for this behavior to generalize to
virtual module sources.
Copy link

@Jamesernator Jamesernator Jul 19, 2022

Choose a reason for hiding this comment

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

Could we design the virtual module API such that we could actually do so? For example Node's experimental loader API has a mechanism through which to communicate evaluation between threads (as the loader is always run on a separate thread from the main code).

We could do something similar by having a remoteExecute hook that returns a SourceTextModule (or script?) to evaluate on the remote host e.g. something like:

class JsonModuleSource {
     bindings = [{ export: "default" }];

     #text = text;
     constructor(text) {
         this.#text = text;
         JSON.parse(text); // check parsing is successful
     }

    remoteExecute() {
        // Supposing the extension of having module blocks return
        // Module objects we could provide a module block that will be evaluated
        // on whatever thread this source is sent to
        return {
            // data to be structured cloned and sent along with the module
            data: { text: this.#text },
            module: module {
                export function execute(namespace, data) {
                    namespace.default = JSON.parse(data.text);
                }
            },
        };
    }
}

const jsonModule = new Module(new JsonModuleSource("{}"));
const worker = new Worker("./worker.js");

// the module gets the executeRemote data cloned so we have everything on the other
// thread in order to be able to execute the module on any thread
worker.postMessage({ jsonModule });

Copy link
Member Author

Choose a reason for hiding this comment

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

@Jamesernator I would like to continue this conversation, possibly on another issue.

One of the motivations for separating Module Source Record from Module Record is that this will allow us to share module sources between threads. ModuleSource instances are thin objects with a [[ModuleSource]] internal slot. So, a thread pool could conceivably fetch, compile, analyze a module graph in parallel and trivially transfer the compiled sources to the main thread for instantiation.

That would work for everything except virtual module sources. The underlying text could be fetched on a thread, but it’d have to be transferred and lifted into an instance on the main.

And, the other limitation is that between workers, virtual modules can only be transferred by a user-code protocol that knows about all the virtual module types. That is really just a generalization of the underlying truth though: structured clone would have to know about every kind of module source it can transfer too.

Copy link
Collaborator

@erights erights left a comment

Choose a reason for hiding this comment

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

@caridy :

I'm fine merging this as it is, and we can iterate over it.

Given the timing, I'm ok with that too. I'll just register that I am unhappy with the design in the current text re import.meta and passing referrer info. But as @caridy says, we'll keep iterating.

Certainly, prior to tc39 tomorrow, merging this PR puts the proposal into vastly better shape than it is right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants