Backport: Set dynamic import callback #17823

jkrems · 2017-12-22T16:57:00Z

Original PR: module: Set dynamic import callback #15713
This includes cherry picks of the following commits:
I went for the nuclear option and backed out everything connected to host defined options. This feature isn't used in our current implementation of dynamic import (or modules in general).

Checklist

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

Affected core subsystem(s)

deps, module

/cc @nodejs/v8 @bmeck @MylesBorins

jkrems · 2017-12-22T17:00:08Z

Looking what V8 CI thinks of this: https://ci.nodejs.org/job/node-test-commit-v8-linux/1133/

jkrems · 2017-12-22T17:52:12Z

Guess I forgot to add some detail but I'd like to get some feedback on the overall direction before I bother figuring out how to run the V8 tests from inside of the node tree.

stderr:
#
# Fatal error in v8::HandleScope::CreateHandle()
# Cannot create a handle without a HandleScope
#

Received signal 6

==== C stack trace ===============================

 [0x0000011880ef]
 [0x2b16a2911330]
 [0x2b16a2b55c37]
 [0x2b16a2b59028]
 [0x00000085c01f]
 [0x00000090dfb1]
 [0x000000b7aa99]
 [0x000000b4d768]
 [0x000000b4da95]
 [0x00000092e129]
 [0x000000610854]
 [0x000000508870]
 [0x000000509398]
 [0x2b16a2b40f45]
 [0x00000044b540]
[end of stack trace]
Command: /home/iojs/build/workspace/node-test-commit-v8-linux/nodes/benchmark/v8test/v8test/deps/v8/out/x64.release/cctest --random-seed=-262725084 test-api/ScriptOrigin --nohard-abort

MylesBorins · 2018-01-14T05:42:39Z

Can you please include 11ebaff

jkrems · 2018-01-15T01:49:51Z

@MylesBorins Tried to pick but it looks like it's already there: https://github.com/nodejs/node/blob/v9.x-staging/doc/api/esm.md#supported

jkrems · 2018-01-15T17:42:58Z

Started another test-commit-v8 run just in case the previous failures vanished for some reason. Still mystified by the error itself. :(

https://ci.nodejs.org/job/node-test-commit-v8-linux/1161/

targos · 2018-01-17T09:06:30Z

@jkrems is that ready for a new CI run?

jkrems · 2018-01-17T15:40:12Z

I’m still not sure where exactly the error comes from. *Somewhere* in here is a missing handle scope afaict.

…

On Wed, Jan 17, 2018 at 1:07 AM Michaël Zasso ***@***.***> wrote: @jkrems <https://github.com/jkrems> is that ready for a new CI run? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17823 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAio9HpUU6PPuB4HeyD2eIKeQwugztnWks5tLbhHgaJpZM4RLO9F> .

detrohutt · 2018-01-31T21:56:40Z

Any chance of this making it into the next release? I'm currently having to use the nightly build which is causing me to have to do several workarounds in my workflow. Is there anything I could do to help, with no knowledge of the node codebase (like just general debugging)?

jkrems · 2018-01-31T22:02:49Z

I'm about to be out all weekend so the earliest I can continue looking into this would be next week. But the last two times I kinda ran out of ideas and I don't have a working setup to recreate it locally yet. :(

detrohutt · 2018-01-31T22:57:53Z

Ok no problem thanks for the quick response.

MylesBorins · 2018-02-01T00:13:19Z

@jkrems could you include #18387
specifically ba3d55a

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

currently if you want to use dynamic import you must use both the `--experimental-modules` and the `--harmony-dynamic-imports` flags. Chrome is currently shipping dynamic import unflagged, the flag only remains in V8 to guard embedders who have not set the appropriate callback from throwing an unhandled rejection when the feature is used. As such it is reasonable to enable the flag by default for `--experimental-modules` PR-URL: nodejs#18387 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

jkrems · 2018-02-01T18:40:50Z

Added ba3d55a and rebased.

jkrems · 2018-02-01T18:52:33Z

Had one last idea of what could have gone wrong when patching up the V8 commit: https://ci.nodejs.org/job/node-test-commit-v8-linux/1192/

jkrems · 2018-02-01T19:31:52Z

test-commit-v8 passes now! 🎉

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

@MylesBorins Afaict this is good to go now.

detrohutt · 2018-02-01T21:04:42Z

@jkrems good work! I see 9.5.0 was just released yesterday. I’ve looked at the node release history and can’t see a pattern to minor releases. Any idea when this may get released now?

jkrems · 2018-02-01T21:32:13Z

Looks like there's been 9.x release about twice a month. I would suspect that minor releases happen when there "happens" to be semver-minor changes. So, my best guess is that this could be part of a 9.x release in the next 2-3 weeks.

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

currently if you want to use dynamic import you must use both the `--experimental-modules` and the `--harmony-dynamic-imports` flags. Chrome is currently shipping dynamic import unflagged, the flag only remains in V8 to guard embedders who have not set the appropriate callback from throwing an unhandled rejection when the feature is used. As such it is reasonable to enable the flag by default for `--experimental-modules` Backport-PR-URL: #17823 PR-URL: #18387 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

MylesBorins · 2018-02-20T21:30:15Z

landed in 3725d4c...d89f310