joyent/node — pull request #5918 by domenic (2013-07-27 05:15:09+00:00)
Title: vm2 WIP

Wanted to get some feedback on this approach before I take it further. See #3042 for what we're trying to fix and previous attempts in that direction.

### General approach

- Transplant [contextify](https://github.com/brianmcd/contextify/) directly into Node core, replacing `node_script.cc`. I'm using [this version](https://gist.github.com/domenic/6068120) as the base; it strips out the `#ifdef`s necssary for supporting old-Node.
- Added, in C++, a `ContextifyScript#RunInThisContext` method, since that functionality is significantly different from and simpler than `ContextifyScript#RunInContext`.
- Given `ContextifyScript` and `ContextifyContext` constructors from C++, implement the entire `vm.js` module in JS.

### Changes from existing vm module

There's a some changes from the existing vm, so tell me which of these you need fix. Probably all of them, for backward compatibility, but if we want to clean up the API now's the time:

- Context objects are no longer represented by their globals, i.e. when you do `vm.createContext(initSandbox)` you no longer get a global back. Instead you get a `ContextifyContext` object which has `run` and `getGlobal` methods. To fix this I'd need to maintain a weak map of globals to their `ContextifyContext`s. Which would be really easy if we were compiling with `--harmony-collections`, but will have to involve C++ if we can't, which is sad because `vm.js` is so nice and JS-only as-is.
- I got rid of the timeout params because they were not documented and seemed hard to implement. There are tests for them though? What should I do?
- The `.name`s of the scripts are now `ContextifyScript` instead of `NodeScript`. I personally think this is a nice homage, but understand how it could be confusing.
- script methods are no longer bound and no longer type-check their `this`. (Do we do this for all C++ objects?)
- `require('vm')` is no longer the `Script` constructor, but instead just an object. There is no longer a `require('vm').Script` (or `require('vm').Script.Script.Script.Script`...).

### Other Probable TODOs

- Make contextify's coding style match the rest of the code. (Help appreciated in what are the major mismatches.)
- Try to kill `node_script.cc` and `process.bind('evals')` in favor of this new thing. It's used in a surprising number of places though. (Can I just use `require('vm')` instead of `process.binding('evals')`?)

/cc @isaacs @Benvie @brianmcd