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

deps: cherry-pick 39d546a from upstream v8 #20016

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Apr 13, 2018

Two commits here:

  1. [api] introduce v8::Value::IsModuleNamespaceObject (v8/v8@39d546a)
  2. util: introduce isModuleNamespaceObject (ff9eba9e4dfd99f5d5d3f4963e3703a3128cc6f0)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 13, 2018
@@ -70,6 +70,7 @@ Felix Geisendörfer <haimuiba@gmail.com>
Filipe David Manana <fdmanana@gmail.com>
Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Geoffrey Garside <ggarside@gmail.com>
Gus Caplan <me@gus.host>
Copy link
Member

Choose a reason for hiding this comment

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

Congrats on your first V8 commit. :-)

@devsnek devsnek added util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency. dont-land-on-v4.x and removed lib / src Issues and PRs related to general changes in the lib or src directory. dont-land-on-v8.x labels Apr 13, 2018
@devsnek
Copy link
Member Author

devsnek commented Apr 13, 2018

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

what do i put in the node-test-commit-v8-linux config

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 13, 2018

Your username and branch name. There's a way to specify the PR but I always forget the syntax - it's something like refs/pulls/20016/head. Won't hurt to try, worst case the checkout fails and you try again.

@devsnek
Copy link
Member Author

devsnek commented Apr 13, 2018

@MylesBorins
Copy link
Contributor

@devsnek is this ABI breaking?
Congrats on V8 contrib 🎉

@devsnek
Copy link
Member Author

devsnek commented Apr 14, 2018

@MylesBorins it just adds a new method to v8::Value, that's not "breaking" right? i'll add semver-minor for new util.types method i suppose

@devsnek devsnek added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 14, 2018
@devsnek devsnek force-pushed the backport/v8-39d546a24022b62b00aedf7b556ac6c9e2306aab branch from ff9eba9 to d870fad Compare April 15, 2018 14:43
@devsnek
Copy link
Member Author

devsnek commented Apr 15, 2018

one last ci after rebase https://ci.nodejs.org/job/node-test-pull-request/14303/

@targos
Copy link
Member

targos commented Apr 15, 2018

Please add Refs: https://github.com/v8/v8/commit/39d546a24022b62b00aedf7b556ac6c9e2306aab to the commit message and capitalize V8

Original commit message:

    [api] introduce v8::Value::IsModuleNamespaceObject

    This allows an embedder to check if a Value is a module namespace object.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Idffceff451dd5f5c6a53d4cb3ce02c1c2c5b653c
    Reviewed-on: https://chromium-review.googlesource.com/1011762
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#52597}

Refs: v8/v8@39d546a
@devsnek
Copy link
Member Author

devsnek commented Apr 15, 2018

@targos done, imma just wait for these tests to finish before pushing

@devsnek
Copy link
Member Author

devsnek commented Apr 15, 2018

failure unrelated, another ChannelClosedException

@devsnek devsnek force-pushed the backport/v8-39d546a24022b62b00aedf7b556ac6c9e2306aab branch from d870fad to 8daa1c9 Compare April 15, 2018 19:44
devsnek added a commit that referenced this pull request Apr 15, 2018
Original commit message:

    [api] introduce v8::Value::IsModuleNamespaceObject

    This allows an embedder to check if a Value is a module namespace object.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Idffceff451dd5f5c6a53d4cb3ce02c1c2c5b653c
    Reviewed-on: https://chromium-review.googlesource.com/1011762
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#52597}

Refs: v8/v8@39d546a

PR-URL: #20016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
devsnek added a commit that referenced this pull request Apr 15, 2018
PR-URL: #20016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@devsnek
Copy link
Member Author

devsnek commented Apr 15, 2018

landed as 73f13ba...c974f1b

jasnell pushed a commit that referenced this pull request Apr 17, 2018
This was missed in a previous PR

PR-URL: #20105
Refs: #20016
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request May 1, 2018
Original commit message:

    [api] introduce v8::Value::IsModuleNamespaceObject

    This allows an embedder to check if a Value is a module namespace object.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Idffceff451dd5f5c6a53d4cb3ce02c1c2c5b653c
    Reviewed-on: https://chromium-review.googlesource.com/1011762
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#52597}

Refs: v8/v8@39d546a

PR-URL: nodejs#20016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
Original commit message:

    [api] introduce v8::Value::IsModuleNamespaceObject

    This allows an embedder to check if a Value is a module namespace object.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Idffceff451dd5f5c6a53d4cb3ce02c1c2c5b653c
    Reviewed-on: https://chromium-review.googlesource.com/1011762
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#52597}

Refs: v8/v8@39d546a

PR-URL: nodejs#20016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
PR-URL: nodejs#20016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jdalton
Copy link
Member

jdalton commented May 8, 2018

Probably should have only exposed this with --experimental-modules.
I believe this may be the only API related to experimental modules that is now public (unflagged).
It's currently undocumented in Node v10.

@MylesBorins
Copy link
Contributor

@jdalton this has now landed in V8 so we can't really flag the feature. We could flag the util, do you think that makes sense?

@jdalton
Copy link
Member

jdalton commented May 8, 2018

Yep, makes sense. l'm talking about the exposed user facing bit not v8 internals bit.

@devsnek
Copy link
Member Author

devsnek commented May 8, 2018

I think it should just be documented, not flagged. there is nothing experimental about this api.

@jdalton
Copy link
Member

jdalton commented May 8, 2018

It's only useful with experimental modules or experimental vm-modules.
Shrug, it's a wart for the foreseeable future (until ESM lands).

@devsnek
Copy link
Member Author

devsnek commented May 8, 2018

without esm landing, vm.Module and c++ add-ons exist which can both produce module namespaces. I can open a pr to add the docs during my lunch break later today.

@jdalton
Copy link
Member

jdalton commented May 8, 2018

The vm.Module is behind the --experimental-vm-modules flag
(the unit test in this commit requires it).

A flag, any flag, is fine... just probably not public unflagged at the moment.

Update:

Related PR doc which makes it officially exposed is #20616. It's marked as fast-track so if there are objections to it in favor of flagging you might raise them sooner than later.

targos pushed a commit to targos/node that referenced this pull request May 31, 2018
Original commit message:

    [api] introduce v8::Value::IsModuleNamespaceObject

    This allows an embedder to check if a Value is a module namespace object.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Idffceff451dd5f5c6a53d4cb3ce02c1c2c5b653c
    Reviewed-on: https://chromium-review.googlesource.com/1011762
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#52597}

Refs: v8/v8@39d546a

PR-URL: nodejs#20016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 1, 2018
Original commit message:

    [api] introduce v8::Value::IsModuleNamespaceObject

    This allows an embedder to check if a Value is a module namespace object.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Idffceff451dd5f5c6a53d4cb3ce02c1c2c5b653c
    Reviewed-on: https://chromium-review.googlesource.com/1011762
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#52597}

Refs: v8/v8@39d546a

PR-URL: #20016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 1, 2018
Original commit message:

    [api] introduce v8::Value::IsModuleNamespaceObject

    This allows an embedder to check if a Value is a module namespace object.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Idffceff451dd5f5c6a53d4cb3ce02c1c2c5b653c
    Reviewed-on: https://chromium-review.googlesource.com/1011762
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#52597}

Refs: v8/v8@39d546a

PR-URL: #20016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2018
@MylesBorins
Copy link
Contributor

opting to not land on v8.x

Please feel free to change the label... this may make sense to include in a larger backport of modules / vm support fo 8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants