fix: pass vm scrip options through

[SimenB]

In order to prevent scripts from potentially running forever, it'd be great to be able to pass a timeout to it.

Docs: https://nodejs.org/api/vm.html#vm_script_runincontext_contextifiedsandbox_options

[TimothyGu]

Can you update the corresponding documentation in README?

[SimenB]

Done! 🙂

[domenic]

Why not use the options argument to the Script constructor?

[SimenB]

Oh, that's a very good point! That works for my use case then. Feel free to close if you don't want this 🙂

[SimenB]

@domenic actually, that doesn't seem to work.

This snippet hangs forever:

const vm = require('vm');

const script = new vm.Script('while(true){}', {timeout: 1000});

script.runInContext(vm.createContext());

While this snippet throws a timeout error:

const vm = require('vm');

const script = new vm.Script('while(true){}');

script.runInContext(vm.createContext(), {timeout: 1000});

Not sure if it's a bug in Node, or something with how scripts are executed.

This is running node 8.11.1

[SimenB]

To my eyes it looks like new Script doesn't care about the timeout, and the docs are wrong:

https://github.com/nodejs/node/blob/9461f327f50c9885508392938053a62985d13259/lib/vm.js#L45-L52

[TimothyGu]

@SimenB Could you file an issue at https://github.com/nodejs/node? I'd be happy to look at it over there!

[SimenB]

Done! nodejs/node#20982

[SimenB]

This is apparently faulty docs, so the PR is back on the table, I think :)

[domenic]

Can we add a test to the runVMScript tests?

[domenic]

Some nits:

• I've tried to stick with adding parens for function/method names, so runVMScript().
• I've tried to stick to Node.js instead of Node.

Also maybe we should update the header to runVMScript(script[, options])? Dunno, could go either way.

[SimenB]

@domenic sorry, I completely forgot about this! I've rebased, addressed your comments on the docs and added a test 🙂

[domenic]

Great stuff, thank you!