vm: add dynamic import support #22381

devsnek · 2018-08-17T23:14:58Z

adds importModuleDynamically callback option to vm.SourceTextModule and vm.Script constructors.
splits contextify::ContextifyScript into node_contextify.h and node_contextify.cc

Closes #19363
Refs #19570

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

nodejs-github-bot · 2018-08-17T23:15:00Z

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/583/pipeline

addaleax · 2018-08-23T22:56:06Z

better docs and tests coming as soon(tm)

Is this WIP or not so much? Either way, it needs a rebase…

devsnek · 2018-08-24T00:12:40Z

@addaleax should be good to go now.

also ping @nodejs/vm

jdalton · 2018-08-24T00:24:13Z

@devsnek Can you summarize the changes to contextify.

devsnek · 2018-08-24T21:28:52Z

@vsemozhetbyt did that 👍

CI https://ci.nodejs.org/job/node-test-pull-request/16737/

vsemozhetbyt · 2018-08-24T23:28:46Z

doc/api/vm.md

+  * `importModuleDynamically` {Function} Called during evaluation of this
+    module when `import()` is called.
+    * `specifier` {string} Specifier passed to `import()`.
+    * `module`. {vm.SourceTextModule} This `vm.SourceTextModule` instance.


Nit: the period between `module` and {vm.SourceTextModule} seems not traditional in this place.

vsemozhetbyt · 2018-08-24T23:28:59Z

doc/api/vm.md

+  * `importModuleDynamically` {Function} Called during evaluation of this
+    module when `import()` is called.
+    * `specifier` {string} Specifier passed to `import()`.
+    * `script`. {vm.Script} This `vm.Script` instance.


devsnek · 2018-08-25T00:33:37Z

CI https://ci.nodejs.org/job/node-test-pull-request/16745/

guybedford

Great work.

lib/internal/modules/esm/translators.js

guybedford · 2018-08-25T09:07:14Z

src/env.h

-  std::unordered_multimap<int, loader::ModuleWrap*> module_map;
+  std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
+  std::unordered_multimap<uint32_t, loader::ModuleWrap*> id_to_module_map;
+  std::unordered_multimap<uint32_t, contextify::ContextifyScript*>


This seems like a lot of maps to me - are you sure this is the simplest way to do this?

it's a lot of maps. it's also the only way. We've talked to V8 about this before, but we're never going to be able to identify modules except by js primitive values so I use number -> wrap maps.

Why is this a multimap? Shouldn't an ID uniquely identify a module? ~~Furthermore, since the ID is always incremented one by one maybe some other data structure would be more appropriate than a hash table? A vector maybe.~~ I guess you need to be able to erase elements. In that case a hash table is fine.

guybedford · 2018-08-25T09:09:07Z

src/module_wrap.cc

+
+  int type = options->Get(0).As<Number>()->Int32Value();
+  if (type == ScriptType::kScript) {
+    uint32_t id = options->Get(1).As<Number>()->Uint32Value();


This isn't my area at all, so can't really comment - but I'd be interested if you could explain what this ID is here?

it comes from those counters I added to env

TimothyGu · 2018-08-25T17:19:52Z

lib/internal/process/esm_loader.js

@@ -3,40 +3,44 @@
 const { internalBinding } = require('internal/bootstrap/loaders');
 const {
  setImportModuleDynamicallyCallback,
-  setInitializeImportMetaObjectCallback
+  setInitializeImportMetaObjectCallback,


Would you be able to revert these changes? I understand that trailing comma is your preferred style but they don't have to accompany everything else in this already large PR.

TimothyGu · 2018-08-25T17:20:59Z

doc/api/vm.md

+  * `importModuleDynamically` {Function} Called during evaluation of this
+    module when `import()` is called.
+    * `specifier` {string} Specifier passed to `import()`.
+    * `module` {vm.SourceTextModule} This `vm.SourceTextModule` instance.


Could you be consistent with initalizeImportMeta in the documentation style here? I.e., having the parameters be part of the description rather than as a sublist.

TimothyGu · 2018-08-25T17:21:22Z

lib/internal/errors.js

@@ -862,6 +862,8 @@ E('ERR_V8BREAKITERATOR',
 // This should probably be a `TypeError`.
 E('ERR_VALID_PERFORMANCE_ENTRY_TYPE',
  'At least one valid performance entry type is required', Error);
+E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING',
+  'A dynamic import callback was not specified.', Error);


TimothyGu · 2018-08-25T17:49:29Z

src/node_contextify.cc

+      Number::New(isolate, loader::ScriptType::kScript));
+  host_defined_options->Set(1, Number::New(isolate, contextify_script->id()));
+
+  env->id_to_script_map.emplace(contextify_script->id(), contextify_script);


Perhaps this could be moved into the ContextifyScript constructor?

TimothyGu · 2018-08-25T17:50:36Z

src/module_wrap.cc

@@ -157,7 +171,10 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
  ModuleWrap* obj = new ModuleWrap(env, that, module, url);
  obj->context_.Reset(isolate, context);

-  env->module_map.emplace(module->GetIdentityHash(), obj);
+  env->hash_to_module_map.emplace(module->GetIdentityHash(), obj);
+  env->id_to_module_map.emplace(obj->id(), obj);


Ditto about moving this into the ModuleWrap constructor.

test/parallel/test-vm-module-dynamic-import.js

+      importModuleDynamically(specifier, wrap) {
+        assert.strictEqual(specifier, 'foo');
+        assert.strictEqual(wrap, s);
+        return foo.namespace;


TimothyGu · 2018-08-25T17:59:46Z

src/module_wrap.cc

+
+  Local<Value> object;
+
+  int type = options->Get(0).As<Number>()->Int32Value();


You cannot assume that options always a) exists, b) has two items, c) both of which are integers, as embedders might be creating their own scripts and modules.

The Int32Value overload that does not take a context is deprecated and removed from the latest version of V8.

TimothyGu · 2018-08-25T18:00:17Z

src/module_wrap.cc

-  if (maybe_result.ToLocal(&result)) {
-    return handle_scope.Escape(result.As<Promise>());
+  if (import_callback->Call(
+        context, v8::Undefined(iso), 2, import_args).ToLocal(&result)) {


arraysize(import_args)

TimothyGu · 2018-08-25T18:00:42Z

src/module_wrap.cc

-    return handle_scope.Escape(result.As<Promise>());
+  if (import_callback->Call(
+        context, v8::Undefined(iso), 2, import_args).ToLocal(&result)) {
+    return result.As<Promise>();


Add a CHECK(result->IsPromise()); here.

TimothyGu · 2018-08-25T18:05:17Z

doc/api/vm.md

@@ -171,6 +171,10 @@ const contextifiedSandbox = vm.createContext({ secret: 42 });
    to initialize the `import.meta`. This function has the signature `(meta,
    module)`, where `meta` is the `import.meta` object in the `Module`, and
    `module` is this `vm.SourceTextModule` object.
+  * `importModuleDynamically` {Function} Called during evaluation of this
+    module when `import()` is called.


This gives me close to no information on how to use this option. I know when it's called and what information I'm given, but… what does it do? If I don't specify this, is import() going to work? Is there a default importModuleDynamically? What am I supposed to return? Examples?

lib/internal/modules/esm/translators.js

doc/api/vm.md

guybedford

Was feeling some odd deja vu here that I'd said all my comments before... then remembered this is the reincarnation of #19717 :)

guybedford · 2018-08-26T13:04:45Z

test/parallel/test-vm-module-dynamic-import.js

+      importModuleDynamically: common.mustCall((specifier, wrap) => {
+        assert.strictEqual(specifier, 'foo');
+        assert.strictEqual(wrap, s);
+        return foo.namespace;


Can we have some cases of invalid returns being tested here? What is the error?

there is no limit on what you can return.

This seems very dangerous (and I'm sure you'll agree!).

As an alternative could we not do something like:

Only support a vm.Module instance from importModuleDynamically

To delegate to loader.import have a special "loader" return value or indicator for this function.

I don't really see any danger, could you fill me in on how you're approaching this?

Well say I'm using this API for the first time - I will likely just return an object here and see that it works and make that the pattern I use in my code for dynamic imports. We shouldn't be encouraging a model that dynamic import is just "anything", but rather that it is another linking process in the graph. There's a big difference between exposing a hook that can return anything and a hook that goes through the same module logic again. So I'm just saying we should probably have it take vm.Module instances as that is the module logic.

For example, error handling should be the same between environments - in this approach an error throw in dynamic import resolution isn't cached. That is a spec violation.

As an alternative we could make the user return an object with a property pointing to the namespace object. This way we could typecheck the return value and avoid the whole thenable business until it is necessary.

If we do check the type I would like it to only be an "is namespace" check, because returning a SourceTextModule instance doesn't guarantee that the SourceTextModule instance is in the correct state to be giving out namespaces, so i think its better to explicitly force the user to get that far. It's also worth mentioning that checking if a value is a module namespace is rather expensive because it's a native call.

Yes a namespace check is fine.

So the way the spec defines dynamic import is through a host hook that returns a Module record - https://tc39.github.io/proposal-dynamic-import/#sec-hostimportmoduledynamically.

This is done so that the error can then be taken from the module record itself.

As mentioned, I am very hesitant about the implementation of this function as breaks the spec on error handling here.

To try and clarify some of the cases here:

Having import('x') twice in the same source can return different modules each time it is called. This isn't explicitly a spec invariant, so could be seen as acceptable.

It is possible for import('x') to provide an error that does not correspond to an instantiation or evaluation error on a module record.

There are no guarantees that import('x') when resolving to the same module twice, will return the same cached error

With the namespace type check:

It is possible for import('x') to not be able to resolve a valid module that is a thenable

Without the namespace type check:

It is possible for import('x') to return any JavaScript binding without invoking any module loading algorithms at all.

If we mandated a return value type of "Module | "defaultLoader" () (not final API but the API suggestion, then the cases become:

Once the promise has resolved to the "module" | default loader indicator string, we then get correct module errors and caching. Errors before this resolution that are not instantiation errors can still be spec-invariant, but we close this gap somewhat.

It is possible to resolve thenable modules

It is not possible to resolve an arbitrary JS binding without resolving through a Module

joyeecheung · 2018-10-04T20:33:37Z

src/node_contextify.h

+
+  inline uint32_t id() { return id_; }
+
+  SET_NO_MEMORY_INFO()


You have these macros called twice, that should be causing complilation failures

devsnek · 2018-10-05T16:46:45Z

CI https://ci.nodejs.org/job/node-test-pull-request/17649/

devsnek · 2018-10-06T18:07:15Z

rebuild of containered https://ci.nodejs.org/job/node-test-commit-linux-containered/7619/ and https://ci.nodejs.org/job/node-test-commit-linux-containered/7620/ because checking boxes is hard

PR-URL: nodejs#22381 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

devsnek · 2018-10-06T22:34:42Z

landed in 4c37df7

targos · 2018-10-07T09:37:06Z

Should this be backported to v10.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.

PR-URL: #22381 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

PR-URL: nodejs#22381 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Backport-PR-URL: #25421 PR-URL: #22381 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

devsnek added vm Issues and PRs related to the vm subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Aug 17, 2018

devsnek requested review from bmeck and jkrems August 17, 2018 23:14

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 17, 2018

devsnek force-pushed the feature/vm-dynamic-import branch 2 times, most recently from e93c142 to 81cc223 Compare August 19, 2018 01:48

devsnek force-pushed the feature/vm-dynamic-import branch 2 times, most recently from 67b5f6f to 580fb8a Compare August 24, 2018 00:12

This comment has been minimized.

Sign in to view

devsnek force-pushed the feature/vm-dynamic-import branch from 580fb8a to 51e65e9 Compare August 24, 2018 14:49

vsemozhetbyt reviewed Aug 24, 2018

View reviewed changes

devsnek force-pushed the feature/vm-dynamic-import branch from 51e65e9 to b746fbd Compare August 25, 2018 00:33

jasnell approved these changes Aug 25, 2018

View reviewed changes

guybedford reviewed Aug 25, 2018

View reviewed changes

devsnek force-pushed the feature/vm-dynamic-import branch from b746fbd to b331674 Compare August 25, 2018 17:55

TimothyGu reviewed Aug 25, 2018

View reviewed changes

devsnek force-pushed the feature/vm-dynamic-import branch 3 times, most recently from 6df2a36 to 5ad9d8a Compare August 25, 2018 23:46

guybedford reviewed Aug 26, 2018

View reviewed changes

lib/internal/modules/esm/translators.js Show resolved Hide resolved

guybedford reviewed Aug 26, 2018

View reviewed changes

doc/api/vm.md Show resolved Hide resolved

guybedford reviewed Aug 26, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

joyeecheung mentioned this pull request Oct 2, 2018

CI failures: 20180927-20181002 nodejs/reliability#18

Closed

devsnek force-pushed the feature/vm-dynamic-import branch 2 times, most recently from c742e31 to ce01c43 Compare October 4, 2018 18:47

joyeecheung reviewed Oct 4, 2018

View reviewed changes

devsnek force-pushed the feature/vm-dynamic-import branch from ce01c43 to 6cf2208 Compare October 4, 2018 21:20

This comment has been minimized.

Sign in to view

devsnek force-pushed the feature/vm-dynamic-import branch from 6cf2208 to 948fcb4 Compare October 5, 2018 14:44

vm: add dynamic import support

4c37df7

PR-URL: nodejs#22381 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

devsnek force-pushed the feature/vm-dynamic-import branch from 948fcb4 to 4c37df7 Compare October 6, 2018 22:33

devsnek merged commit 4c37df7 into nodejs:master Oct 6, 2018

devsnek deleted the feature/vm-dynamic-import branch October 6, 2018 22:34

targos added the backport-requested-v10.x label Oct 7, 2018

jasnell pushed a commit that referenced this pull request Oct 17, 2018

vm: add dynamic import support

6487f07

PR-URL: #22381 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

joyeecheung mentioned this pull request Nov 24, 2018

src: compile native modules and their code cache in C++ #24221

Closed

3 tasks

joyeecheung mentioned this pull request Dec 22, 2018

module: use compileFunction over Module.wrap #21573

Closed

4 tasks

joyeecheung mentioned this pull request Jan 9, 2019

[10.x] vm: add dynamic import support #25421

Closed

BethGriggs mentioned this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

BethGriggs removed the backport-requested-v10.x label Jun 14, 2019

SimenB mentioned this pull request Apr 21, 2020

vm.compileFunction is missing importModuleDynamically #31860

Closed


		Local<Value> object;

		int type = options->Get(0).As<Number>()->Int32Value();

vm: add dynamic import support #22381

vm: add dynamic import support #22381

Conversation

devsnek commented Aug 17, 2018 • edited Loading

Checklist

nodejs-github-bot commented Aug 17, 2018

addaleax commented Aug 23, 2018

devsnek commented Aug 24, 2018

jdalton commented Aug 24, 2018

This comment has been minimized.

devsnek commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Aug 25, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

TimothyGu Aug 25, 2018 • edited Loading

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

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

guybedford 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

guybedford Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

devsnek commented Oct 5, 2018

devsnek commented Oct 6, 2018 • edited Loading

devsnek commented Oct 6, 2018

targos commented Oct 7, 2018

devsnek commented Aug 17, 2018 •

edited

Loading

TimothyGu Aug 25, 2018 •

edited

Loading

TimothyGu Aug 25, 2018 •

edited

Loading

TimothyGu Aug 25, 2018 •

edited

Loading

TimothyGu Aug 25, 2018 •

edited

Loading

guybedford Aug 27, 2018 •

edited

Loading

devsnek commented Oct 6, 2018 •

edited

Loading