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

module: Set dynamic import callback #15713

Closed
wants to merge 1 commit into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Oct 1, 2017

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

@nodejs-github-bot 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 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
Copy link
Contributor Author

jkrems commented Oct 1, 2017

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.

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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 jkrems requested a review from bmeck October 1, 2017 17:56
Environment* env = Environment::GetCurrent(context);
HandleScope handle_scope(iso);

Local<Function> import_callback =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

You could use common.crashOnUnhandledRejection() instead.

@@ -0,0 +1,11 @@
// Flags: --experimental-modules --harmony-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.

'use strict'?

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

Choose a reason for hiding this comment

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

Does the handle scope needs to be escapable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}));

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

Choose a reason for hiding this comment

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

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

@@ -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)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jkrems jkrems Oct 2, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (path.isAbsolute(referrer)) {
return getURLFromFilePath(referrer);
}
return referrer;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly.

@jkrems
Copy link
Contributor Author

jkrems commented Oct 2, 2017

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 Fishrock123 self-assigned this Oct 2, 2017
@Fishrock123
Copy link
Contributor

assigning in a futile attempt to remember this

@bmeck
Copy link
Member

bmeck commented Oct 3, 2017

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

@@ -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>());
Copy link
Contributor Author

@jkrems jkrems Oct 3, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@jkrems jkrems changed the title [WIP] module: Set dynamic import callback module: Set dynamic import callback Oct 3, 2017
@jkrems
Copy link
Contributor Author

jkrems commented Oct 3, 2017

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
Copy link
Contributor Author

jkrems commented Oct 7, 2017

@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
Copy link
Member

targos commented Oct 7, 2017

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

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

jkrems commented Oct 7, 2017

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

jkrems pushed a commit to jkrems/node that referenced this pull request Feb 1, 2018
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]>
jkrems pushed a commit to jkrems/node that referenced this pull request Feb 1, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
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 MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins added a commit that referenced this pull request Feb 21, 2018
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
MylesBorins added a commit that referenced this pull request Feb 21, 2018
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
MylesBorins added a commit that referenced this pull request Feb 22, 2018
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
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
MylesBorins pushed a commit that referenced this pull request May 23, 2018
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]>
MylesBorins pushed a commit that referenced this pull request May 23, 2018
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
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
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 MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.