Skip to content

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 <[email protected]>
Filipe David Manana <[email protected]>
Franziska Hinkelmann <[email protected]>
Geoffrey Garside <[email protected]>
Gus Caplan <[email protected]>
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

devsnek added 2 commits April 15, 2018 08:15
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 <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    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 <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#52597}

Refs: v8/v8@39d546a

PR-URL: #20016
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
devsnek added a commit that referenced this pull request Apr 15, 2018
PR-URL: #20016
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#52597}

Refs: v8/v8@39d546a

PR-URL: nodejs#20016
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#52597}

Refs: v8/v8@39d546a

PR-URL: nodejs#20016
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
PR-URL: nodejs#20016
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@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 <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#52597}

Refs: v8/v8@39d546a

PR-URL: nodejs#20016
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#52597}

Refs: v8/v8@39d546a

PR-URL: #20016
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#52597}

Refs: v8/v8@39d546a

PR-URL: #20016
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@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