8000 Backport: Set dynamic import callback by hybrist · Pull Request #17823 · nodejs/node · GitHub
  • [go: up one dir, main page]
    Skip to content

    Backport: Set dynamic import callback #17823

    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.

    Sign up for GitHub

    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

    Jump to bottom
    Closed
    hybrist wants to merge 5 commits into from
    Closed

    Backport: Set dynamic import callback #17823

    hybrist wants to merge 5 commits into from

    Conversation

    @hybrist
    Copy link
    Contributor
    @hybrist hybrist commented Dec 22, 2017 •
    edited
    Loading
    • 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

    @hybrist hybrist added backported-to-v9.x wip Issues and PRs that are still a work in progress. labels Dec 22, 2017
    @nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v9.x labels Dec 22, 2017
    @hybrist hybrist mentioned this pull request Dec 22, 2017
    Closed
    9 tasks
    @hybrist
    Copy link
    Contributor Author
    hybrist commented Dec 22, 2017

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

    @hybrist
    Copy link
    Contributor Author
    hybrist commented Dec 22, 2017

    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 MylesBorins force-pushed the v9.x-staging branch 2 times, most recently from 7177a88 to ffc2659 Compare January 10, 2018 06:45
    @MylesBorins
    Copy link
    Contributor
    MylesBorins commented Jan 14, 2018

    Can you please include 11ebaff

    @MylesBorins MylesBorins mentioned this pull request Jan 14, 2018
    Closed
    2 tasks
    @hybrist hybrist force-pushed the jk-backport-dynamic-import branch from 38b4df4 to dfb78ce Compare January 15, 2018 01:42
    @hybrist
    Copy link
    Contributor Author
    hybrist commented Jan 15, 2018

    @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

    @hybrist
    Copy link
    Contributor Author
    hybrist commented Jan 15, 2018

    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/

    @hybrist hybrist force-pushed the jk-backport-dynamic-import branch from dfb78ce to c2167dd Compare January 15, 2018 20:34
    @targos
    Copy link
    Member
    targos commented Jan 17, 2018

    @jkrems is that ready for a new CI run?

    @targos targos mentioned this pull request Jan 17, 2018
    Closed
    @hybrist
    Copy link
    Contributor Author
    hybrist commented Jan 17, 2018 via email

    @detrohutt
    Copy link
    detrohutt commented Jan 31, 2018

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

    @hybrist
    Copy link
    Contributor Author
    hybrist commented Jan 31, 2018

    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
    Copy link
    detrohutt commented Jan 31, 2018

    Ok no problem thanks for the quick response.

    @MylesBorins
    Copy link
    Contributor
    MylesBorins commented Feb 1, 2018

    @jkrems could you include #18387
    specifically ba3d55a

    Jan Krems and others added 4 commits February 1, 2018 10:39
    deps: cherry-pick dbfe4a49d8 from upstream V8
    cef8dc9 
    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 <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#47724}

PR-URL: nodejs#16889
Refs: v8/v8@dbfe4a4
Refs: nodejs#15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    module: Set dynamic import callback
    8102dbb 
    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 <timothygu99@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
    fixup! deps: cherry-pick dbfe4a49d8 from upstream V8
    b369026
    @MylesBorins
    module: enable dynamic import flag for esmodules
    e9af79c 
    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 <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    @hybrist hybrist force-pushed the jk-backport-dynamic-import branch from a820e06 to e9af79c Compare February 1, 2018 18:40
    @hybrist
    Copy link
    Contributor Author
    hybrist commented Feb 1, 2018

    Added ba3d55a and rebased.

    @hybrist
    Copy link
    8000
    Contributor Author
    hybrist commented Feb 1, 2018 •
    edited
    Loading

    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/

    @hybrist
    Copy link
    Contributor Author
    hybrist commented Feb 1, 2018

    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
    Copy link
    detrohutt commented Feb 1, 2018

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

    @hybrist
    Copy link
    Contributor Author
    hybrist commented Feb 1, 2018

    Looks like there's been 9.x release about twice 6D47 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.

    @hybrist hybrist removed the wip Issues and PRs that are still a work in progress. label Feb 7, 2018
    MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
    @MylesBorins
    deps: cherry-pick dbfe4a49d8 from upstream V8
    11566fe 
    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 <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

Backport-PR-URL: #17823
PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
    @MylesBorins
    module: Set dynamic import callback
    00d5422 
    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 <timothygu99@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
    MylesBorins added a commit that referenced this pull request Feb 20, 2018
    @MylesBorins
    module: enable dynamic import flag for esmodules
    d89f310 
    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
9E79

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 <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    @MylesBorins
    Copy link
    Contributor
    MylesBorins commented Feb 20, 2018

    landed in 3725d4c...d89f310

    @hybrist hybrist deleted the jk-backport-dynamic-import branch February 20, 2018 22:37
    MylesBorins pushed a commit that referenced this pull request May 23, 2018
    @MylesBorins
    deps: cherry-pick dbfe4a49d8 from upstream V8
    29f49e9 
    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 <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

Backport-PR-URL: #17823
PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    MylesBorins pushed a commit that referenced this pull request May 23, 2018
    @MylesBorins
    module: Set dynamic import callback
    c4c468b 
    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 <timothygu99@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
    MylesBorins added a commit that referenced this pull request May 23, 2018
    @MylesBorins
    module: enable dynamic import flag for esmodules
    a0c7df8 
    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 <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
    @MylesBorins
    deps: cherry-pick dbfe4a49d8 from upstream V8
    0af02ab 
    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 <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

Backport-PR-URL: #17823
PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
    @MylesBorins
    module: Set dynamic import callback
    73a2d91 
    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 <timothygu99@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
    MylesBorins added a commit that referenced this pull request Jun 14, 2018
    @MylesBorins
    module: enable dynamic import flag for esmodules
    d6f8630 
    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 <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    rvagg pushed a commit that referenced this pull request Aug 16, 2018
    @rvagg
    deps: cherry-pick dbfe4a49d8 from upstream V8
    656ceea 
    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 <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

Backport-PR-URL: #17823
PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    rvagg pushed a commit that referenced this pull request Aug 16, 2018
    @rvagg
    module: Set dynamic import callback
    d7192c4 
    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 <timothygu99@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
    rvagg pushed a commit that referenced this pull request Aug 16, 2018
    @MylesBorins @rvagg
    module: enable dynamic import flag for esmodules
    0f83f25 
    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 <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @jasnell jasnell jasnell approved these changes

    +2 more reviewers

    @TimothyGu TimothyGu TimothyGu approved these changes

    @XadillaX XadillaX XadillaX approved these changes

    Reviewers whose approvals may not affect merge requirements

    Assignees

    No one assigned

    Labels

    lib / src Issues and PRs related to general changes in the lib or src directory.

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    8 participants

    @hybrist @MylesBorins @targos @detrohutt @jasnell @TimothyGu @XadillaX @nodejs-github-bot
    0