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

vm: filename option to vm.runInNewContext() ignored #27055

Closed
bnoordhuis opened this issue Apr 2, 2019 · 2 comments
Closed

vm: filename option to vm.runInNewContext() ignored #27055

bnoordhuis opened this issue Apr 2, 2019 · 2 comments
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@bnoordhuis
Copy link
Member

This should work according to the docs but currently doesn't:

require('vm').runInNewContext(`throw Error('boom')`, {filename: 'boom.js'})

It shows up as evalmachine.<anonymous> instead of boom.js.

Confirmed with master and all release lines. It's apparently not a recent regression.

Regression test:

diff --git a/test/parallel/test-vm-run-in-new-context.js b/test/parallel/test-vm-run-in-new-context.js
index 51577668cf..6c8cc921a7 100644
--- a/test/parallel/test-vm-run-in-new-context.js
+++ b/test/parallel/test-vm-run-in-new-context.js
@@ -72,14 +72,13 @@ global.gc();
 fn();
 // Should not crash
 
-{
+for (const cb of [(code, filename) => vm.runInNewContext(code, {}, filename),
+                  (code, filename) => vm.runInNewContext(code, { filename })]) {
   // Verify that providing a custom filename as a string argument works.
   const code = 'throw new Error("foo");';
   const file = 'test_file.vm';
 
-  assert.throws(() => {
-    vm.runInNewContext(code, {}, file);
-  }, (err) => {
+  assert.throws(() => cb(code, file), (err) => {
     const lines = err.stack.split('\n');
 
     assert.strictEqual(lines[0].trim(), `${file}:1`);
@bnoordhuis bnoordhuis added vm Issues and PRs related to the vm subsystem. v6.x labels Apr 2, 2019
@bnoordhuis
Copy link
Member Author

Looking at it from another perspective, maybe it's just a documentation issue? From vm.md:

## vm.runInNewContext(code[, sandbox[, options]])

To me that reads as saying the sandbox argument can be omitted but I think it should be interpreted as "sandbox can be omitted only when options is"? It's not super clear, you can't really divine it from the description either.

@bnoordhuis
Copy link
Member Author

Thinking about it more, I don't see a way of making it work the way I expected it to (no way to know if the second arg is the sandbox or the options object in a two arg call) so I'll go ahead and close this out and think about how to improve the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

1 participant