vm: add support for import.meta to Module #19277

punteek · 2018-03-11T02:14:58Z

Fixes: #18570

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

TimothyGu · 2018-03-11T02:15:38Z

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

devsnek

generally looks good 👍, just a few notes

devsnek · 2018-03-11T02:22:27Z

lib/internal/process/modules.js

@@ -19,7 +23,20 @@ function normalizeReferrerURL(referrer) {
 }

 function initializeImportMetaObject(wrap, meta) {
-  meta.url = wrap.url;
+  const vmModule = wrapToModuleMap.get(wrap);


could you just simplify this to const initializeImportMeta = wrapToFnMap.get(wrap)?

you could also initalise an undefined property in module_wrap and check wrap.property !== undefined which should be faster than a weakmap check

How does const initializeImportMeta = wrapToFnMap.get(wrap) serve as a simplification?

devsnek · 2018-03-11T02:23:33Z

lib/internal/process/modules.js

+const {
+  initImportMetaMap,
+  wrapToModuleMap
+} = require('internal/vm/Module-common');


you can just export these maps from internal/vm/module, and if for some reason this file is kept please rename it to module_common.js to keep with the rest of our files.

I agree, I can move the maps to Module.js.

devsnek · 2018-03-11T02:24:23Z

lib/internal/bootstrap_node.js

@@ -108,7 +108,8 @@
        'DeprecationWarning', 'DEP0062', startup, true);
    }

-    if (process.binding('config').experimentalModules) {
+    if (process.binding('config').experimentalModules ||
+        process.binding('config').experimentalVMModules) {
      process.emitWarning(


this warning should still only show up for --experimental-modules

This means that the original way the warning was written is ok?

@punteek, I think @devsnek meant

if (experimentalModules || experimentalVMModules) { if (experimentalModules) process.emitWarning(...); NativeModule.require(...); }

devsnek · 2018-03-11T02:44:47Z

lib/internal/process/modules.js

+    meta.url = wrap.url;
+  } else {
+    const initializeImportMeta = initImportMetaMap.get(vmModule);
+    if (initializeImportMeta === undefined) {


you can change this to if (initializeImportMeta !== undefined) { /* call method */ }

The reason I currently have a check for undefined is that we would like nothing to happen in the case that no callback is provided.

i meant

if (initializeImportMeta !== undefined) { initializeImportMeta(meta, vmModule); } // no else, does the same thing as your code but without an empty case

devsnek · 2018-03-11T03:03:53Z

doc/api/vm.md


 Creates a new ES `Module` object.

+*Note*: Properties assigned to the `import.meta` object that are objects may


i think people are already used to this just from using vm, might not need this example

I think I would prefer to keep the example for now.

devsnek · 2018-03-24T14:22:40Z

@punteek are you still interested in working on this?

punteek · 2018-03-25T02:19:08Z

Hey! Yes I'd still like to get this update working. I have some questions regarding your suggested changes which you can see above.

punteek · 2018-03-29T06:01:29Z

I've updated some of the discussed changes.

devsnek · 2018-03-29T13:34:14Z

lgtm after a rebase against master

devsnek · 2018-03-29T23:25:07Z

@punteek you'll need to rebase your branch against master. git rebase master --preserve-merges should do it, and then you can safely drop that last merge commit you made.

punteek · 2018-03-30T00:04:09Z

Sorry, I did not see your comment until just now. I tried fixing it on my own, does everything look correct?

devsnek · 2018-03-30T00:41:58Z

@punteek your branch still has lib/internal/bootstrap_node.js, once you get rid of that you should be good to go 👍

punteek · 2018-03-30T01:30:52Z

Alright, it should be gone.

devsnek · 2018-03-30T02:59:14Z

@punteek sorry to keep doing this to you 😄 it seems you also have lib/internal/vm/Modules.js and lib/internal/process/modules.js still in your branch as well.

punteek · 2018-03-30T05:26:03Z

No problem, I should have caught it the first time. Those files should be gone too now.

TimothyGu · 2018-03-30T06:37:22Z

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

Fixes: nodejs#18570

punteek · 2018-03-31T04:11:54Z

I found additional errors in importing due to file name changes. These should be fixed now.

TimothyGu · 2018-03-31T14:37:09Z

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

devsnek · 2018-04-01T00:59:22Z

landed in 07ba914. congratulations @punteek on your first contribution 🎉

Fixes: #18570 PR-URL: #19277 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

punteek · 2018-04-01T05:18:31Z

Thanks to both @TimothyGu and @devsnek, you both made the process very clear for me!

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 11, 2018

TimothyGu requested a review from devsnek March 11, 2018 02:15

TimothyGu added vm Issues and PRs related to the vm subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 11, 2018

devsnek suggested changes Mar 11, 2018

View reviewed changes

devsnek reviewed Mar 11, 2018

View reviewed changes

MylesBorins mentioned this pull request Mar 20, 2018

rocketchat crashes with segmentation fault since nodejs 8.10 #19274

Closed

punteek force-pushed the import-meta-vm branch from 088b886 to 866b1ee Compare March 29, 2018 05:58

punteek force-pushed the import-meta-vm branch 2 times, most recently from 3753c02 to 3b6fb05 Compare March 30, 2018 00:00

punteek force-pushed the import-meta-vm branch from 3b6fb05 to 62889c5 Compare March 30, 2018 01:04

punteek force-pushed the import-meta-vm branch from 62889c5 to aa96f15 Compare March 30, 2018 05:24

TimothyGu approved these changes Mar 30, 2018

View reviewed changes

vm: add support for import.meta to Module

0fe4202

Fixes: nodejs#18570

punteek force-pushed the import-meta-vm branch from aa96f15 to 0fe4202 Compare March 30, 2018 08:42

devsnek approved these changes Mar 31, 2018

View reviewed changes

devsnek closed this Apr 1, 2018

devsnek pushed a commit that referenced this pull request Apr 1, 2018

vm: add support for import.meta to Module

07ba914

Fixes: #18570 PR-URL: #19277 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

targos added dont-land-on-v4.x labels Apr 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: add support for import.meta to Module #19277

vm: add support for import.meta to Module #19277

punteek commented Mar 11, 2018

TimothyGu commented Mar 11, 2018

devsnek left a comment

devsnek Mar 11, 2018 •

edited

Loading

punteek Mar 25, 2018

devsnek Mar 11, 2018 •

edited

Loading

punteek Mar 25, 2018

devsnek Mar 11, 2018

punteek Mar 25, 2018

TimothyGu Mar 25, 2018

devsnek Mar 11, 2018

punteek Mar 25, 2018

devsnek Mar 25, 2018

devsnek Mar 11, 2018

punteek Mar 25, 2018

devsnek commented Mar 24, 2018

punteek commented Mar 25, 2018

punteek commented Mar 29, 2018

devsnek commented Mar 29, 2018

devsnek commented Mar 29, 2018 •

edited

Loading

punteek commented Mar 30, 2018

devsnek commented Mar 30, 2018

punteek commented Mar 30, 2018

devsnek commented Mar 30, 2018

punteek commented Mar 30, 2018

TimothyGu commented Mar 30, 2018

punteek commented Mar 31, 2018

TimothyGu commented Mar 31, 2018

devsnek commented Apr 1, 2018 •

edited

Loading

punteek commented Apr 1, 2018


		Creates a new ES `Module` object.

		Note: Properties assigned to the `import.meta` object that are objects may

vm: add support for import.meta to Module #19277

vm: add support for import.meta to Module #19277

Conversation

punteek commented Mar 11, 2018

Checklist

TimothyGu commented Mar 11, 2018

devsnek left a comment

Choose a reason for hiding this comment

devsnek Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Mar 24, 2018

punteek commented Mar 25, 2018

punteek commented Mar 29, 2018

devsnek commented Mar 29, 2018

devsnek commented Mar 29, 2018 • edited Loading

punteek commented Mar 30, 2018

devsnek commented Mar 30, 2018

punteek commented Mar 30, 2018

devsnek commented Mar 30, 2018

punteek commented Mar 30, 2018

TimothyGu commented Mar 30, 2018

punteek commented Mar 31, 2018

TimothyGu commented Mar 31, 2018

devsnek commented Apr 1, 2018 • edited Loading

punteek commented Apr 1, 2018

devsnek Mar 11, 2018 •

edited

Loading

devsnek Mar 11, 2018 •

edited

Loading

devsnek commented Mar 29, 2018 •

edited

Loading

devsnek commented Apr 1, 2018 •

edited

Loading