module: Set dynamic import callback #15713

jkrems · 2017-10-01T17:52:20Z

This is an initial implementation to support dynamic import in
both scripts and modules. It's off by default since support for
dynamic import is still flagged in V8. Without setting the V8 flag,
this code won't be executed.

Because of missing support in the V8 APIs, we can't support
importing into the proper context yet. Without further changes this
might allow code to break out of the context it's running in.

/cc @bmeck

TODO

Add test case to ensure import from "random contexts" is ignored
~~Clean error handling when calling import() before the callback is set up~~ (can't think of a scenario where this might happen)
Cleanup code for handling import from CommonJS scripts
~~Potentially add explicit error handling for failing import handler call~~ (just preventing a crash for now)
Ensure behavior matches browsers where appropriate (e.g. eval & friends)

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)

module

jkrems · 2017-10-01T17:53:57Z

This is using the 6.1 V8 API for the module callback which will change slightly in 6.2. But I expect the implementation to be fairly easy to adjust once we get there.

jkrems · 2017-10-01T17:54:48Z

.eslintignore

@@ -3,6 +3,7 @@ lib/punycode.js
 test/addons/??_*/
 test/fixtures
 test/tmp*/
+test/es-module/test-esm-dynamic-import.js


Afaict our ESLint doesn't support dynamic import (and won't while it's still in stage 3).

jkrems · 2017-10-01T17:56:20Z

lib/internal/loader/Loader.js

+
+ static registerImportDynamicallyCallback(loader) {
+ setImportModuleDynamicallyCallback(async (referrer, specifier) => {
+ const normalizedBase = /^file:\/\//.test(referrer) ?


This hack allows both CommonJS and ESM to use dynamic import and it's "guessing" which one it is. There's definitely cleaner ways to achieve this and this might break horribly in cases where the referrer is neither (e.g. vm.runIn*Context).

I would add a path.isAbsolute() check to the non-URL code path just to make sure it is what we expect, and throw an error otherwise. vm.runInContext() + friends have a way to specify file path as well through the filename option.

jkrems · 2017-10-01T17:58:28Z

src/module_wrap.cc

+ Environment* env = Environment::GetCurrent(context);
+ HandleScope handle_scope(iso);
+
+ Local<Function> import_callback =


There's currently an unhandled error case when dynamic import is used in an environment that doesn't have the callback defined (yet).

TimothyGu

Some superficial comments. Can't spot any major problems with the implementation.

TimothyGu · 2017-10-01T18:02:36Z

test/es-module/test-esm-dynamic-import.js

+ // Can't deepStrictEqual because ns isn't a normal object
+ assert.deepEqual(ns, { default: true });
+ }))
+ .then(null, err => { process.nextTick(() => { throw err; }); });


You could use common.crashOnUnhandledRejection() instead.

TimothyGu · 2017-10-01T18:02:43Z

test/es-module/test-esm-dynamic-import.js

@@ -0,0 +1,11 @@
+// Flags: --experimental-modules --harmony-dynamic-import


'use strict'?

TimothyGu · 2017-10-01T18:03:10Z

src/module_wrap.cc

+ const FunctionCallbackInfo<Value>& args) {
+ Isolate* iso = args.GetIsolate();
+ Environment* env = Environment::GetCurrent(args);
+ EscapableHandleScope handle_scope(iso);


Does the handle scope needs to be escapable?

Don't think so, bad copy&paste. Thanks!

TimothyGu

Other than #15713 (comment), this LGTM as far as I'm concerned.

bmeck · 2017-10-01T20:15:47Z

src/module_wrap.cc

+ Local<String> specifier) {
+ Isolate* iso = context->GetIsolate();
+ // TODO(jkrems): ensure proper context is used
+ Environment* env = Environment::GetCurrent(context);


if we could perform the check that only the main context (env.context_) gets import() handling setup, that would be good. We don't have a public exposed way in vm currently to setup things for creating new contexts.

I made it an explicit error condition when someone tries to use dynamic import from another context. Also added some test cases for it.

jkrems · 2017-10-01T22:27:30Z

test/es-module/test-esm-dynamic-import.js

+ }));
+
+// Succeeds because it's got an valid base url
+Promise.resolve(vm.runInThisContext(`import("${relativePath}")`, {


Note: This will introduce a de-facto global access to require that doesn't exist right now. E.g. random code running in vm.runInThisContext will gain access to all node APIs.

Before:

% ./node <(echo "console.log(require('vm').runInThisContext('require(\"fs\")'));") evalmachine.<anonymous>:1 require("fs") ^ ReferenceError: require is not defined # ...

After:

% ./node --experimental-modules --harmony <(echo "require('vm').runInThisContext('import(\"fs\")').then(console.log);") (node:29002) ExperimentalWarning: The ESM module loader is experimental. { default: { constants: # ...

jkrems · 2017-10-01T22:35:30Z

lib/internal/loader/resolveRequestUrl.js

@@ -80,6 +80,10 @@ const resolveRequestUrl = (baseURLOrString, specifier) => {
 const baseURL = normalizeBaseURL(baseURLOrString);
 let url = search(specifier, baseURL);

+ if (url.protocol === 'node:' && NativeModule.nonInternalExists(url.pathname)) {


@bmeck This was needed to support import('fs'). The problem was that the URL this function produces (node:fs) wasn't accepted as an input, leading to fragility ("better not try to normalize URLs twice!"). With this change, normalizing an already normalized URL should be safe.

can you explain this a bit more, right now import("node:fs") should throw an error from user code.

Gotcha. The general problem is that I resolve the url before calling loader.import and as a side effect node:fs makes it into first level of importing. I think I can rewrite that to never pass through loader.import or, alternatively, make loader.import take a different referrer/base url. Which would you prefer?

guybedford · 2017-10-02T11:33:50Z

lib/internal/loader/Loader.js

+
+ static registerImportDynamicallyCallback(loader) {
+ setImportModuleDynamicallyCallback(async (referrer, specifier) => {
+ const normalizedBase = normalizeReferrerURL(referrer);


I take it this referrer path context is provided directly from V8 for scripts? What does it become for eval and vm.runInThisContext cases?

Yep, it's 1:1 the ScriptOrigin#url as provided by V8. For vm.runInThisContext it's "evalmachine.anonymous" by default but can be overridden using the filename option.

For both direct and indirect eval referrer is undefined - nice find! Will add test cases and make sure they fail cleanly.

Updated with test cases that document the behavior for eval: 3eaf5fe

Is this the final stance v8 is taking or will this API be changing at all do you know? If it is pretty much set, then I guess we should just adjust the resolver to handle undefined referrer as defaulting to the standard base referrer.

@guybedford this is a bug, V8 6.1 implements import() but 6.2 is the recommended implementations. See https://tc39.github.io/ecma262/#sec-getactivescriptormodule , which will be given instead of a string. So the C++ API changes a bit as well.

@bmeck that seems to be more sensible, thanks for the link.

guybedford · 2017-10-02T11:42:32Z

lib/internal/loader/Loader.js

+ if (path.isAbsolute(referrer)) {
+ return getURLFromFilePath(referrer);
+ }
+ return referrer;


Is this check needed because the referrer is a file path for scripts and a URL for modules?

Yep, exactly.

jkrems · 2017-10-02T15:44:17Z

Alright, cleaned up the tests to not repeat the checks every single time and introduced loadNamespaceForRequest which allows import("node:fs") to fail and import("fs") to succeed while still sharing most of the code with import statements. 6b76466

Fishrock123 · 2017-10-02T17:14:01Z

assigning in a futile attempt to remember this

bmeck · 2017-10-03T13:45:50Z

+1 with the knowledge that we will have to update when V8 6.2 lands.

jkrems · 2017-10-03T16:05:23Z

src/module_wrap.cc

@@ -517,7 +517,7 @@ static MaybeLocal<Promise> ImportModuleDynamically(Local<Context> context,
 Local<Value> error = v8::Exception::Error(
 OneByteString(iso, "invalid_context"));
 if (resolver->Reject(context, error).IsJust()) {
- return resolver.As<Promise>();
+ return handle_scope.Escape(resolver.As<Promise>());


@TimothyGu I admit that I'm not 100% familiar with proper HandleScope handling - but I assumed that I had to go back to an EscapableHandleScope because I'm now creating a new object (Promise::Resolver::New above) and return it in this code path? Happy to revert if this isn't necessary. :)

@jkrems Yep I believe this is necessary now. Thanks for checking!

jkrems · 2017-10-03T16:08:05Z

Added one last small commit to clean up MaybeLocal and scope escaping from my understanding of those. Removed WIP because this should be good to land now (with @bmeck's disclaimer that the handling of referrer and C++ signatures will change once 6.2 lands).

jkrems · 2017-10-07T18:55:02Z

@MylesBorins I'm hesitant to land this in case 6.2 is "just around the corner". Do we have a good feeling yet if 6.2 will land before end of October?

targos · 2017-10-07T19:04:20Z

According to the Chromium release schedule, V8 6.2 will become stable on Oct 17th. We should be landing it shortly after.

jkrems · 2017-10-07T19:05:37Z

Alright, will wait for 6.2 and adjust before merge. Shouldn't affect most of the implementation and might prevent annoying failures/conflicts.

Original commit message: Introduce ScriptOrModule and HostDefinedOptions This patch introduces a new container type ScriptOrModule which provides the name and the host defined options of the script/module. This patch also introduces a new PrimitivesArray that can hold Primitive values, which the embedder can use to store metadata. The HostDefinedOptions is passed to V8 through the ScriptOrigin, and passed back to the embedder through HostImportModuleDynamically for module loading. Bug: v8:5785, v8:6658, v8:6683 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a Reviewed-on: https://chromium-review.googlesource.com/622158 Reviewed-by: Adam Klein <[email protected]> Reviewed-by: Georg Neis <[email protected]> Commit-Queue: Sathya Gunasekaran <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#47724} PR-URL: nodejs#16889 Refs: v8/v8@dbfe4a4 Refs: nodejs#15713 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This is an initial implementation to support dynamic import in both scripts and modules. It's off by default since support for dynamic import is still flagged in V8. Without setting the V8 flag, this code won't be executed. This initial version does not support importing into vm contexts. PR-URL: nodejs#15713 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Bradley Farias <[email protected]>

Original commit message: Introduce ScriptOrModule and HostDefinedOptions This patch introduces a new container type ScriptOrModule which provides the name and the host defined options of the script/module. This patch also introduces a new PrimitivesArray that can hold Primitive values, which the embedder can use to store metadata. The HostDefinedOptions is passed to V8 through the ScriptOrigin, and passed back to the embedder through HostImportModuleDynamically for module loading. Bug: v8:5785, v8:6658, v8:6683 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a Reviewed-on: https://chromium-review.googlesource.com/622158 Reviewed-by: Adam Klein <[email protected]> Reviewed-by: Georg Neis <[email protected]> Commit-Queue: Sathya Gunasekaran <[email protected]> Cr-Commit-Position: refs/heads/master@{#47724} Backport-PR-URL: #17823 PR-URL: #16889 Refs: v8/v8@dbfe4a4 Refs: #15713 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This is an initial implementation to support dynamic import in both scripts and modules. It's off by default since support for dynamic import is still flagged in V8. Without setting the V8 flag, this code won't be executed. This initial version does not support importing into vm contexts. Backport-PR-URL: #17823 PR-URL: #15713 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Bradley Farias <[email protected]>

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

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

Original commit message: Introduce ScriptOrModule and HostDefinedOptions This patch introduces a new container type ScriptOrModule which provides the name and the host defined options of the script/module. This patch also introduces a new PrimitivesArray that can hold Primitive values, which the embedder can use to store metadata. The HostDefinedOptions is passed to V8 through the ScriptOrigin, and passed back to the embedder through HostImportModuleDynamically for module loading. Bug: v8:5785, v8:6658, v8:6683 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a Reviewed-on: https://chromium-review.googlesource.com/622158 Reviewed-by: Adam Klein <[email protected]> Reviewed-by: Georg Neis <[email protected]> Commit-Queue: Sathya Gunasekaran <[email protected]> Cr-Commit-Position: refs/heads/master@{#47724} Backport-PR-URL: #17823 PR-URL: #16889 Refs: v8/v8@dbfe4a4 Refs: #15713 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This is an initial implementation to support dynamic import in both scripts and modules. It's off by default since support for dynamic import is still flagged in V8. Without setting the V8 flag, this code won't be executed. This initial version does not support importing into vm contexts. Backport-PR-URL: #17823 PR-URL: #15713 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Bradley Farias <[email protected]>

MylesBorins · 2018-05-23T18:48:50Z

backported to v8.x in 453077f...a0c7df8

Original commit message: Introduce ScriptOrModule and HostDefinedOptions This patch introduces a new container type ScriptOrModule which provides the name and the host defined options of the script/module. This patch also introduces a new PrimitivesArray that can hold Primitive values, which the embedder can use to store metadata. The HostDefinedOptions is passed to V8 through the ScriptOrigin, and passed back to the embedder through HostImportModuleDynamically for module loading. Bug: v8:5785, v8:6658, v8:6683 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a Reviewed-on: https://chromium-review.googlesource.com/622158 Reviewed-by: Adam Klein <[email protected]> Reviewed-by: Georg Neis <[email protected]> Commit-Queue: Sathya Gunasekaran <[email protected]> Cr-Commit-Position: refs/heads/master@{#47724} Backport-PR-URL: #17823 PR-URL: #16889 Refs: v8/v8@dbfe4a4 Refs: #15713 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This is an initial implementation to support dynamic import in both scripts and modules. It's off by default since support for dynamic import is still flagged in V8. Without setting the V8 flag, this code won't be executed. This initial version does not support importing into vm contexts. Backport-PR-URL: #17823 PR-URL: #15713 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Bradley Farias <[email protected]>

Original commit message: Introduce ScriptOrModule and HostDefinedOptions This patch introduces a new container type ScriptOrModule which provides the name and the host defined options of the script/module. This patch also introduces a new PrimitivesArray that can hold Primitive values, which the embedder can use to store metadata. The HostDefinedOptions is passed to V8 through the ScriptOrigin, and passed back to the embedder through HostImportModuleDynamically for module loading. Bug: v8:5785, v8:6658, v8:6683 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a Reviewed-on: https://chromium-review.googlesource.com/622158 Reviewed-by: Adam Klein <[email protected]> Reviewed-by: Georg Neis <[email protected]> Commit-Queue: Sathya Gunasekaran <[email protected]> Cr-Commit-Position: refs/heads/master@{#47724} Backport-PR-URL: #17823 PR-URL: #16889 Refs: v8/v8@dbfe4a4 Refs: #15713 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This is an initial implementation to support dynamic import in both scripts and modules. It's off by default since support for dynamic import is still flagged in V8. Without setting the V8 flag, this code won't be executed. This initial version does not support importing into vm contexts. Backport-PR-URL: #17823 PR-URL: #15713 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Bradley Farias <[email protected]>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 1, 2017

jkrems added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. labels Oct 1, 2017

jkrems commented Oct 1, 2017

View reviewed changes

jkrems requested a review from bmeck October 1, 2017 17:56

jkrems commented Oct 1, 2017

View reviewed changes

jkrems force-pushed the jk-dynamic-import branch from da964c1 to c794624 Compare October 1, 2017 18:00

TimothyGu reviewed Oct 1, 2017

View reviewed changes

jkrems force-pushed the jk-dynamic-import branch from c794624 to 4e9744d Compare October 1, 2017 18:11

TimothyGu approved these changes Oct 1, 2017

View reviewed changes

bmeck suggested changes Oct 1, 2017

View reviewed changes

jkrems commented Oct 1, 2017

View reviewed changes

guybedford reviewed Oct 2, 2017

View reviewed changes

Fishrock123 self-assigned this Oct 2, 2017

bmeck approved these changes Oct 3, 2017

View reviewed changes

jkrems commented Oct 3, 2017

View reviewed changes

jkrems changed the title ~~[WIP] module: Set dynamic import callback~~ module: Set dynamic import callback Oct 3, 2017

jkrems added the wip Issues and PRs that are still a work in progress. label Oct 7, 2017

jkrems force-pushed the jk-dynamic-import branch from f3fbc2a to bb74d43 Compare October 7, 2017 21:23

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added dont-land-on-v4.x labels Feb 26, 2018

targos added backported-to-v9.x and removed backport-requested-v9.x labels Mar 24, 2018

MylesBorins added the backported-to-v8.x label May 23, 2018

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

		@@ -0,0 +1,11 @@
		// Flags: --experimental-modules --harmony-dynamic-import

module: Set dynamic import callback #15713

module: Set dynamic import callback #15713

Conversation

jkrems commented Oct 1, 2017 • edited Loading

TODO

Checklist

Affected core subsystem(s)

jkrems commented Oct 1, 2017

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

TimothyGu 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

TimothyGu 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

jkrems Oct 2, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems commented Oct 2, 2017 • edited Loading

Fishrock123 commented Oct 2, 2017

bmeck commented Oct 3, 2017

jkrems Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems commented Oct 3, 2017

jkrems commented Oct 7, 2017

targos commented Oct 7, 2017

jkrems commented Oct 7, 2017

MylesBorins commented May 23, 2018

jkrems commented Oct 1, 2017 •

edited

Loading

jkrems Oct 2, 2017 •

edited

Loading

jkrems commented Oct 2, 2017 •

edited

Loading

jkrems Oct 3, 2017 •

edited

Loading