vm: add module support by devsnek · Pull Request #17560 · nodejs/node

devsnek · 2017-12-09T04:06:58Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

vm, module

bnoordhuis · 2017-12-09T12:51:43Z

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

devsnek · 2017-12-09T12:57:42Z

@bnoordhuis thats what i'm working on rn

devsnek · 2017-12-09T14:55:12Z

@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

Can you undo the whitespace churn in node_contextify.cc? Flattening it is acceptable but not squashed along with functional changes.

bnoordhuis · 2017-12-09T15:07:46Z

src/module_wrap.h

Must be a Persistent, Locals are only valid for the duration of their enclosing HandleScope.

thank you!! i completely overlooked this 😄

devsnek · 2017-12-09T15:48:41Z

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

bnoordhuis · 2017-12-09T16:02:48Z

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

devsnek · 2017-12-10T04:09:35Z

@Trott i think this is not a WIP anymore

addaleax · 2017-12-11T21:01:05Z

doc/api/vm.md

we generally use REPLACEME here because that will be picked up by the release tooling

guybedford · 2017-12-12T10:01:56Z

lib/vm.js

Should we make these generated URL values unique for debugging?

I wasn't sure how to do that based on the input, maybe a counter variable (vm:module:1), or it could just stay vm:module. scripts get evalmachine.<anonymous> in the stack if they don't have a filename

BridgeAR · 2017-12-12T21:45:29Z

@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? 😃

devsnek · 2017-12-13T02:35:13Z

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

TimothyGu · 2017-12-13T07:24:22Z

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:

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

PR-URL: nodejs#17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Adds vm.Module, which wraps around ModuleWrap to provide an interface for developers to work with modules in a more reflective manner. Co-authored-by: Timothy Gu <timothygu99@gmail.com> PR-URL: nodejs#17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Flattens ContextifyContext allows the context interface to be used in other parts of the code base. PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Adds vm.Module, which wraps around ModuleWrap to provide an interface for developers to work with modules in a more reflective manner. Co-authored-by: Timothy Gu <timothygu99@gmail.com> PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Flattens ContextifyContext allows the context interface to be used in other parts of the code base. PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Adds vm.Module, which wraps around ModuleWrap to provide an interface for developers to work with modules in a more reflective manner. Co-authored-by: Timothy Gu <timothygu99@gmail.com> PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Flattens ContextifyContext allows the context interface to be used in other parts of the code base. PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Adds vm.Module, which wraps around ModuleWrap to provide an interface for developers to work with modules in a more reflective manner. Co-authored-by: Timothy Gu <timothygu99@gmail.com> PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) #18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) #18354 - ICU 60.2 bump (Steven R. Loomis) #17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) #16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) #15752 * http2: - add http fallback options to .createServer (Peter Marton) #15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) #16402 * inspector: - --inspect-brk for es modules (Guy Bedford) #18194 * lib: - allow process kill by signal number (Sam Roberts) #16944 * module: - enable dynamic import (Myles Borins) #18387 - dynamic import is now supported (Jan Krems) #15713 * napi: - add methods to open/close callback scope (Michael Dawson) #18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) #17600 * vm: - add support for es modules (Gus Caplan) #17560 PR-URL: #18902

Flattens ContextifyContext allows the context interface to be used in other parts of the code base. PR-URL: nodejs#17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

PR-URL: nodejs#17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Adds vm.Module, which wraps around ModuleWrap to provide an interface for developers to work with modules in a more reflective manner. Co-authored-by: Timothy Gu <timothygu99@gmail.com> PR-URL: nodejs#17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) nodejs#18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) nodejs#18354 - ICU 60.2 bump (Steven R. Loomis) nodejs#17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) nodejs#16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) nodejs#15752 * http2: - add http fallback options to .createServer (Peter Marton) nodejs#15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) nodejs#16402 * inspector: - --inspect-brk for es modules (Guy Bedford) nodejs#18194 * lib: - allow process kill by signal number (Sam Roberts) nodejs#16944 * module: - enable dynamic import (Myles Borins) nodejs#18387 - dynamic import is now supported (Jan Krems) nodejs#15713 * napi: - add methods to open/close callback scope (Michael Dawson) nodejs#18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) nodejs#17600 * vm: - add support for es modules (Gus Caplan) nodejs#17560 PR-URL: nodejs#18902

codebytere · 2018-08-02T17:53:04Z

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

TimothyGu · 2018-08-02T19:33:42Z

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

nodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label Dec 9, 2017

Trott added the wip Issues and PRs that are still a work in progress. label Dec 9, 2017

devsnek force-pushed the feature/vm-modules branch from b5522b4 to e6a8335 Compare December 9, 2017 14:56

bnoordhuis reviewed Dec 9, 2017

View reviewed changes

devsnek force-pushed the feature/vm-modules branch from 66c3ccb to 75f9273 Compare December 9, 2017 15:49

devsnek force-pushed the feature/vm-modules branch 2 times, most recently from 45d91e7 to 53616a7 Compare December 9, 2017 16:42

devsnek changed the title ~~[WIP] vm: add module support~~ vm: add module support Dec 9, 2017

Trott removed the wip Issues and PRs that are still a work in progress. label Dec 10, 2017

addaleax reviewed Dec 11, 2017

View reviewed changes

doc/api/vm.md Outdated

Copy link

Member

addaleax Dec 11, 2017

Choose a reason for hiding this comment

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

we generally use REPLACEME here because that will be picked up by the release tooling

devsnek force-pushed the feature/vm-modules branch from 4b2177b to 3751d2b Compare December 11, 2017 22:33

guybedford reviewed Dec 12, 2017

View reviewed changes

devsnek force-pushed the feature/vm-modules branch 5 times, most recently from 85cd447 to 2b3bb71 Compare December 12, 2017 21:43

devsnek force-pushed the feature/vm-modules branch 4 times, most recently from 3f2458a to 261abf7 Compare December 12, 2017 23:22

devsnek pushed a commit to devsnek/node that referenced this pull request Feb 21, 2018

src: factor out some common vm functions

993f943

PR-URL: nodejs#17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

devsnek mentioned this pull request Feb 21, 2018

[9.x backport] 17560, 18471, 18509 #18891

Closed

2 tasks

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

src: factor out some common vm functions

2e06702

PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

src: factor out some common vm functions

a64917b

PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

devsnek added backported-to-v9.x and removed backport-requested-v9.x labels Feb 21, 2018

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

src: factor out some common vm functions

de3231c

PR-URL: #17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

devsnek removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2018

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

src: factor out some common vm functions

e732ac0

PR-URL: nodejs#17560 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

TimothyGu added the dont-land-on-v8.x label Aug 2, 2018

Uh oh!

Conversation

devsnek commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

bnoordhuis commented Dec 9, 2017

Uh oh!

devsnek commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devsnek commented Dec 9, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 9, 2017

Choose a reason for hiding this comment

Uh oh!

devsnek Dec 9, 2017

Choose a reason for hiding this comment

Uh oh!

devsnek commented Dec 9, 2017

Uh oh!

bnoordhuis commented Dec 9, 2017

Uh oh!

devsnek commented Dec 10, 2017

Uh oh!

addaleax Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

guybedford Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

devsnek Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 12, 2017

Uh oh!

devsnek commented Dec 13, 2017

Uh oh!

TimothyGu commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codebytere commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

devsnek commented Dec 9, 2017 •

edited

Loading

devsnek commented Dec 9, 2017 •

edited

Loading

devsnek Dec 12, 2017 •

edited

Loading

TimothyGu commented Dec 13, 2017 •

edited

Loading

codebytere commented Aug 2, 2018 •

edited

Loading

TimothyGu commented Aug 2, 2018 •

edited

Loading