vm: adds runInModuleContext()

[eljefedelrodeodeljefe]

With regards to nodejs/node-v0.x-archive#9211, this SO question and in order to prevent a signature like the below, I want to propose vm.runInGlobalContext(). This would make the vm API more clear to users, who expect at least one method vm that behaves exactly like their global context.

const vmResult = vm.runInThisContext(require('module').wrap(`
   const http = require('http');
   http.createServer( (request, response) => {
     response.writeHead(200, {'Content-Type': 'text/plain'});
     response.end('Hello World\\n');
   }).listen(8124);

   console.log('Server running at http://127.0.0.1:8124/');
`))(exports, require, module, __filename, __dirname)

[bnoordhuis]

That's not 'run in global context', that's 'run in the context of the vm module'.

[eljefedelrodeodeljefe]

Wouldn't that be the only possibility to have a similar experience like in global context? What would you suggest?

[cjihrig]

Maybe runInModuleContext? Although it's not clear what the name "module" refers to.

[eljefedelrodeodeljefe]

I tried just not wrapping it, but this but again throw ReferenceError: require is not defined

[bnoordhuis]

It's the vm module's module object (as is exports). This function is essentially vm.runInVmContext()...

[eljefedelrodeodeljefe]

Renaming would be fine for me. I am just looking for ways to have similar require behaviour...while not having the signature as in the description of course. Anyhow it would just be sugar.(good -> less doc explanation sugar)

[cjihrig]

Sorry, I meant it's not clear to end users what "module" in the name would refer to. But yea, just passing vm's values through isn't great.

[vkurchatkin]

Sorry, but this implementation is incorrect

[cjihrig]

You might want to take a look at the real module system (lib/module.js, src/node.js, etc.). Also, the vm functions let you specify a filename, which would probably be the correct value for __filename. Not really sure what you'd want to use for __dirname.

[eljefedelrodeodeljefe]

The purpose would be to pass everything from the global scope into the scope of the vm. Implementation-wise I am happy for input. Apart from that, can you advise on any other way to be able to require? I think the above way was derived from a couple of tickets. in the archive repo.

[eljefedelrodeodeljefe]

Reference would be this comment

[vkurchatkin]

The purpose would be to pass everything from the global scope into the scope of the vm

I don't think it's possible

[cjihrig]

None of those variables come from the global scope. Each one has its own unique value in every module. If you want to have access to their values in the current file, you would have to pass them in as arguments.

[eljefedelrodeodeljefe]

@vkurchatkin I don't necessarily want global scope, but basically just use require-module-system.

The use case would be to send a fs.readFileSync('someserver.js') string to a remote client and run the server in the same process and same thread as the client is running in. It should then be able to require http and spawn a server. Calling require though will fail, because the context has no global variable require, without passing it to it. I thought .runInThisContext() would already be doing this, but the open issues would show, that there is confusion.

[vkurchatkin]

I don't necessarily want global scope, but basically just use require-module-system.

it would be confusing. require needs proper file path to work

[cjihrig]

@eljefedelrodeodeljefe I put together vm.runInModuleContext(), in cjihrig@befd2c2. I doubt there will be much interest to have it in core (I'll PR it if there is), but it would do what you're looking for. Unfortunately, it relies on 'internal/module', so it's not very convenient for userland either (although it could be adapted).

[eljefedelrodeodeljefe]

Okay. Edited this especially the name (edit: not updated in docs), since I have a feeling that I didn't make the purpose of it clear enough. I think what you @cjihrig did is exactly what I was writing, but not what I meant. Also I agree that this then shouldn't be in core. However, I still believe that having a bad API like below, over the latter implementation, should be worth the change (really badly). Speaking out of practice, I really can't teach devs require('module')-wrap-iife-bang, just to let them write js in a vm like they write outside of it, namely requiring native modules.

  const vmResult = vm.runInThisContext(require('module').wrap(`
  const http = require('http');
  http.createServer( (request, response) => {
    response.writeHead(200, {'Content-Type': 'text/plain'});
    response.end('Hello World\\n');
  }).listen(8124);

  console.log('Server running at http://127.0.0.1:8124/');

  `))(exports, require, module)

const vm = require('vm')

const vmResult = vm.runInModuleContext(`
  const http = require('http');
  http.createServer( (request, response) => {
    response.writeHead(200, {'Content-Type': 'text/plain'});
    response.end('Hello World\\n');
  }).listen(8124);

  console.log('Server running at http://127.0.0.1:8124/');
`)

[bnoordhuis]

@eljefedelrodeodeljefe When exactly would one use that? The module system used to have an undocumented switch to load each module in a separate vm context but, besides being badly broken for most of its existence, I don't think there really ever was a good use case for. I've never seen one, at least.

(In case you're wondering why the switch existed in the first place: it was added very early on in node's life, for no real reason other than 'because we can'.)

[eljefedelrodeodeljefe]

@bnoordhuis I am hacking on a cluster implementation that would support child_processes, detached local process and remote clients. All of them should be able to register at the master node and get instructions which code to run as string via dgram. The current cluster implementation would only support the child process. The idea was to use vm in order not to use the filesystem and not to spawn just a another process, since I can use the worker process and thread right away.

[eljefedelrodeodeljefe]

So basically doing this meta programming would also be possible with the solution I am proposing right in vm now just doing userland. It's extremely bulky though. I reckoned that this might be really useful for a lot of use cases and people. This might also render the vm module API more friendly to users and hence propagate the use of it.

[vkurchatkin]

This implementation is not really useful. Why would someone want vm's exports or module objects? Why would some want require that is only capable of loading built in modules? You can just use eval if that's what you need

[eljefedelrodeodeljefe]

Well eval() use seems to be an anti-pattern to me in general in js and it's behaviour will likely directly depend on the v8 implementation (#2245). Also It allows no sandboxing and options whatsoever. If I could to this without passing anything I'd be happy. It looks like a hack and probably it is also one, but at least it works and would make meta programming js less bulky. I was really (negatively) surprised about vm not being able to require things, to be honest, since it's a very easy step. Those kind of things make up for a lot of tickets and SO Q&As. So I'd advocate: Even if there is pain in it, we should have it for dev-education purposes.

[vkurchatkin]

Also It allows no sandboxing and options whatsoever. If I could to this without passing anything I'd be happy

sandboxing means to give explicit access to objects. That's exactly what you are trying to avoid.

Even if there is pain in it, we should have it for dev-education purposes.

Have what? Current implementation will only cause more confusion

[eljefedelrodeodeljefe]

Sorry, but I find telling people to do this (below) or eval() really really bad just in order to run some node code:

const vmResult = vm.runInThisContext(require('module').wrap(`
   // code 
`))(exports, require, module)

And still I don't see much pointing against the implementation.

Re: Sandboxing: I am not avoiding it. I just (and this my sole purpose) want to require modules like http. Wherever it might be confusing it might also jsut be the name runInModuleContext which is evenly bad to all the rest. The way vm's API and documentation is currently, I'd rather regard this as a internal module.

[vkurchatkin]

Your implementation is not equivalent to this code. It doesn't give access to current modules exports, require and module. It doesn't give access to local variables ('eval' does). It has nothing to do with sandboxing. If you permit require it basically means you permit everything.

[eljefedelrodeodeljefe]

Yes and that's fine for me. I don't want to share anything, but run a server. Wherever the exports, modules and requires are coming from, I don't mind, since I just want to use them, all security over board. Security in my use case wouldn't be necessary, since trusting code wouldn't be of concern here.

I also have no intimate knowledge of those modules, but this doesn't seem necessary in this context to me. If there is a non-eval way for running node.js code, please feel free to tell me.

[vkurchatkin]

Yes and that's fine for me

That's not a very good reason to include this in core.

Security in my use case wouldn't be necessary, since trusting code wouldn't be of concern here.

So, eval is fine then

[eljefedelrodeodeljefe]

Updated the branch. Apparently it's not necessary to pass the module object.

No, because eval() being generally an js anti-pattern. And for other reasons I was already describing above -.-

Again. Having an API with function signatures like above and on open tickets in the archive repo, is worse then adding a couple of symbols to core, imo.

If there is no consensus, I'll be fine. But this leaves me a little bit in disbelieve, tbh.

[vkurchatkin]

Ok, I'm done arguing. Here are the problems with your code:

Doesn't work:

vm.runInModuleContext(`
   var express = require('express');
 `)

Doesn't work:

var a = 1;
vm.runInModuleContext(`
   console.log(a);
 `)

Doesn't work:

vm.runInModuleContext(`
   exports.foo = 'bar';
 `)

Works, but it shouldn't

vm.runInModuleContext(`
   require('internal/module');
 `)

[eljefedelrodeodeljefe]

See, that is more productive. While case 2 and 3 are out my scope, 4 should not be possible and for, this should be made equivalent:

(while the first works the second does not yet, but I put some work into it)

vm.runInThisContext(require('module').wrap(`
   var express = require('express');
   var app = express();
   app.get('/', function(req, res){
      res.send('hello world');
    });

    app.listen(3000);
 `))(exports, require)


 vm.runInModuleContext(`
    var express = require('express');
    var app = express();
    app.get('/', function(req, res){
       res.send('hello world 2');
     });

     app.listen(3001);
  `)

[eljefedelrodeodeljefe]

@vkurchatkin worked on this. I think the update factors it all in. However one would need to pass it ass options, which might also be a good idea in the end.

vm.runInThisContext(`
  console.log(this);
 `)

let options = {
  passThrough: [ exports, require ]
}

var a = 1;
vm.runInModuleContext(`
  var express = require('express');
  var app = express();
  app.get('/', function(req, res){
    res.send('hello world 2');
  });
  app.listen(3001);

  exports.foo = 'bar'; // -> works
  // require('internal/module'); -> does not work, as expected
  // console.log(a);  -> does not work, but is okay
  console.log(this); // should be same as in runInThisContext()
`, options)

[bnoordhuis]

To be honest, I don't really see the value of this new method if you end up calling vm.runInThisContext() in the end anyway, it's basically non-bareword eval().

You mention you want to use it in a kind of in-memory cluster but I have trouble seeing what it would buy you.

[eljefedelrodeodeljefe]

Hm. Okay, so be it. No, the point would be to run it in another machine. However this was just the opportunity for me to find the vm API really bulky. The proposed change would not be a technical addition, but API enhancement. Yes, the effect is basically eval(), but without all the features vm has and w/o telling js developers they should use something they shouldn't.

This discussion was an attrition, shouldn't have started it. I'll just use the ugly code above.