Is this a work in progress? I ask because I don't see where it actually compiles the module in a different context.

From looking at module_wrap.cc, it uses the creation context of the ModuleWrap, which is the main context. It should enter the new context first before calling ScriptCompiler::CompileModule().

@bnoordhuis thats what i'm working on rn

@bnoordhuis maybe you can take a look at how i pass the context to ModuleWrap, it seems to sigsegv about 50% of the time but i can't trace the issue.

@bnoordhuis i'm pretty sure i had to do that in order to use it from module_wrap, if i'm wrong please let me know.

Right, but then do it in a lead-up commit and save the functional changes for the follow-up commit.

@Trott i think this is not a WIP anymore

@devsnek Without looking at the code right now - it seems like the commits are meant to land individually. Would you be so kind and name them properly and add a description about what that commit does and why it is implemented the way it is? 😃

any thoughts on exposing namespace sorta like run() -> { result, get namespace }

For this feature to be maximally useful, I would rather a lower-level API get exposed. The API should roughly correspond to the spec, thus allowing me to:

1. Change the importing algorithm. For example, my vm.Module-using application may not want to use the default Node.js file-based algorithm, but instead return the script from an HTTP response. This includes setting the importing algorithm for dynamic imports.
2. Execute the three steps separately:
   1. Parse the Module source text
   2. Instantiate the module (which will call the the import algorithm I provide it).
   3. Evaluate the instantiated module.
3. Have access to future ECMAScript features, like being able to set import.meta properties.

There are also other things to consider. For example, if I have two different applications using such an API, they should not conflict with each other. I should also not be allowed to use a Module from one application to fulfill the importing needs of another application.

Some extras that may be useful include the function you mentioned - the function for getting the namespace (corresponding to GetModuleNamespace in spec-speak). But this should come after the core API described above gets established.

It should also be noted that even though the parsing step of the Module doesn't require a Context, at least not in the V8 API, it would probably be a good idea from our API to force a Module to be bound to a specific context from the start to reduce confusion. A special "UnboundModule" could come later.

Those are the requirements. Now here's my take on creating such an API. Note, the following was written without looking into how ModuleWrap is currently implemented, so it may be non-trivial to implement such a set of APIs. But there's no rush.

class ModuleEnvironment {
  // Takes a contextified sandbox object. Check if sandbox is indeed contextified,
  // and throw a TypeError if it is not.
  // Takes an optional `resolveImportedModule` callback, which when called with
  // 1. the parent Module and
  // 2. the specifier string
  // should return a Promise<Module> corresponding to the requested imported
  // module. If it is not specified, the Node.js default resolution algorithm should be
  // used.
  // This could be extended in the future to allow dynamic imports.
  constructor(sandbox, {
    resolveImportedModule = defaultResolver
  } = {}) {
    // ...
  }

  // Optional: create a ModuleEnvironment that bound to the top-level context.
  static createTopEnvironment({
    resolveImportedModule = defaultResolver
  }) {
    // ...
  }

  // Returns the contextified sandbox object.
  // Do not allow changing the context after the ModuleEnvironment has been
  // created.
  // Returns undefined when the context is the top-level context.
  get sandbox() {}
}

class Module {
  // Takes a ModuleEnvironment, a string of source text, and a URL denoting
  // the name of the file. Parses the string as a module (throwing errors if an
  // error occurred).
  // The options object should allow future extension, such as a callback to set
  // properties to `import.meta`.
  constructor(environment, text, { url = 'vm:module' } = {}) {
    // ...
  }

  // Return the bound ModuleEnvironment of this Module. Do NOT allow
  // changing the ModuleEnvironment.
  get environment() {
    // ...
  }

  // Returns one of the possible values for [[Status]] internal slot as listed in
  // https://tc39.github.io/ecma262/#table-38: 'uninstantiated', 'instantiating',
  // 'instantiated', 'evaluating', and 'evaluated'.
  // This should map pretty well to v8::Module::GetStatus().
  get status() {
    // ...
  }

  // Instantiate the Module. Check status is 'uninstantiated'. This will call the
  // resolveImportedModule callback registered with the ModuleEnvironment if
  // this Module has any imports. Check if the Module returned from
  // resolveImportedModule belongs to the same ModuleEnvironment or not.
  // Returns a Promise<void> determining if this operation finished successfully.
  // If it failed, then status should still remain 'uninstantiated' but the Module
  // should be invalidated against future attempts to re-instantiate it, at least until
  // we get evidence that reinstantiation is allowed by the spec and useful in
  // practice.
  instantiate() {
    // ...
  }

  // Evaluate the Module. Check status is 'instantiated'. Return the result of
  // evaluation. If an error occurred during evaluation, invalidate the Module so
  // that it cannot be used for any other purpose. Either way, change status to
  // 'evaluated' at the end.
  evaluate() {
    // ...
  }

  // Return the namespace object of this Module. Check status is 'evaluated'
  // and no error occurred during evaluation.
  getNamespaceObject() {
    // ...
  }
}

This is still a rough draft; whether it is sufficient for the most advanced of use cases or even if it is implementable is still unclear. I'll be asking a few people to look over it, but IMO this is how we should go forward -- with a planning stage where opinions are gathered rather than jumping straight into implementation.

Just checking, is there anything V8 engine specific here in regarding order of operations, that while allowed under spec text of implementation dependent, could cause problems for other engines?
\cc @iamstolis

@jdalton With regards to parsing/instantiation/evaluation, the spec is deterministic and does not allow for any implementation-defined operations (except the algorithm for providing the spec with an imported module given a specifier). Of course, it is entirely possible for implementations to merge some of these steps and separate another into a few individual steps, but as long as there are spec equivalents for those operations this API would be usable. (In fact, there effectively needs to be, as the spec for <script type=module> in HTML uses the same terminology as this API.)

I'd like to land this soon. If there are no more comments I'll land it on Tuesday.

CI: https://ci.nodejs.org/job/node-test-commit/15742/

Can't wait to hammer on it. Thanks @devsnek and @TimothyGu!

Landed in 70277d6...0993fbe 🎉

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

If backported please include #18509

trying to cherry pick 3bf34f2 results in some huge conflicts that i don't know how to handle. if anyone wants to mentor/advise i would welcome it 😄

edit: i think i figured it out, hopefully i didn't break anything while resolving the conflicts

@devsnek 3bf34f27a1, 2033a9f436, and 0993fbe5b2 don't apply cleanly v8.x, can you backport to v8.x or add the dont-land label?

It's an experimental feature, so I'd say let's not land on an LTS branch.