-
Notifications
You must be signed in to change notification settings - Fork 29k
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
vm: add support for import.meta to Module #19277
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good 👍, just a few notes
lib/internal/process/modules.js
Outdated
@@ -19,7 +23,20 @@ function normalizeReferrerURL(referrer) { | |||
} | |||
|
|||
function initializeImportMetaObject(wrap, meta) { | |||
meta.url = wrap.url; | |||
const vmModule = wrapToModuleMap.get(wrap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does const initializeImportMeta = wrapToFnMap.get(wrap) serve as a simplification?
lib/internal/process/modules.js
Outdated
const { | ||
initImportMetaMap, | ||
wrapToModuleMap | ||
} = require('internal/vm/Module-common'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I can move the maps to Module.js.
lib/internal/bootstrap_node.js
Outdated
@@ -108,7 +108,8 @@ | |||
'DeprecationWarning', 'DEP0062', startup, true); | |||
} | |||
|
|||
if (process.binding('config').experimentalModules) { | |||
if (process.binding('config').experimentalModules || | |||
process.binding('config').experimentalVMModules) { | |||
process.emitWarning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this warning should still only show up for --experimental-modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that the original way the warning was written is ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/internal/process/modules.js
Outdated
meta.url = wrap.url; | ||
} else { | ||
const initializeImportMeta = initImportMetaMap.get(vmModule); | ||
if (initializeImportMeta === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can change this to if (initializeImportMeta !== undefined) { /* call method */ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i meant
if (initializeImportMeta !== undefined) {
initializeImportMeta(meta, vmModule);
} // no else, does the same thing as your code but without an empty case
|
||
Creates a new ES `Module` object. | ||
|
||
*Note*: Properties assigned to the `import.meta` object that are objects may |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think people are already used to this just from using vm, might not need this example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer to keep the example for now.
@punteek are you still interested in working on this? |
Hey! Yes I'd still like to get this update working. I have some questions regarding your suggested changes which you can see above. |
I've updated some of the discussed changes. |
lgtm after a rebase against master |
@punteek you'll need to rebase your branch against master. |
3753c02
to
3b6fb05
Compare
Sorry, I did not see your comment until just now. I tried fixing it on my own, does everything look correct? |
@punteek your branch still has |
Alright, it should be gone. |
@punteek sorry to keep doing this to you 😄 it seems you also have |
No problem, I should have caught it the first time. Those files should be gone too now. |
I found additional errors in importing due to file name changes. These should be fixed now. |
landed in 07ba914. congratulations @punteek on your first contribution 🎉 |
Thanks to both @TimothyGu and @devsnek, you both made the process very clear for me! |
Fixes: #18570
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes