Allow user-added globals on the REPL

[TrevorBurnham]

See #1654. The change here is very simple: Rather than specifically listing the globals we want to preserve on the REPL, we list the ones we want to exclude (which are just references to the "real" global/root). This has no effect on the REPL environment whatsoever except when a user requires something that modifies global, either with a command or with the -r flag.

[TrevorBurnham]

Sorry, just tested—this doesn't actually fix the stated issue. Back to the drawing board...

[satyr]

Isn't this unused then?

[TrevorBurnham]

OK, patch modified so globals are reloaded every time you run a command on the REPL. This fixes #1654, but at the cost of making it impossible to override globals defined elsewhere within the REPL. That is, if you require a file with the code

global.a = 7

and then type

> global.a = 23

on the REPL, global.a will be reset to 7 the next time you run a REPL command. So, still working on this.

@satyr Yes, that was a (more minor) mistake. Fixed.

[TrevorBurnham]

I'm starting to think it'd be better to just use the "real" global instead of sandbox. Do you see any downside to this, @michaelficarra? I know it feels intuitively like the two processes should be isolated, but there's a significant implementation cost to doing that and I'm not seeing a real benefit.

[TrevorBurnham]

In fact, there's no way to get the desired behavior while sandbox and global are different objects. If a.js is

global.a = 7

then there's no way for the REPL to determine that a should be 7 after the line

(global.a = 23) && (require './a')

but should be 23 after the line

(require './a') && (global.a = 23)

The REPL can see that both global.a and sandbox.a changed, but it can't tell which change occurred later.

Now, come to think of it, we're already injecting the sandbox's require method (a necessary hack), so perhaps the solution is to get it to require things in such a way that their global points to our sandbox.

Sorry for all the thinking-out-loud today...

[michaelficarra]

... even after all this, I still have no idea why someone would want variables defined globaly in another module to be merged with their global scope when they require it. How does that make any sense? Other modules are supposed to be in an implicit IIFE, right? require should return an object containing the properties attached to exports in the required module. That's all it should do. Any side effects would surely be a bug... right? I feel like I'm the crazy one here.

[TrevorBurnham]

Sure, in principle, you want to keep things isolated. But in practice, attaching things directly to global can be a huge time-saver. There's a reason the global object exists in CommonJS, and it's just weird that the global that's used when you require something from the REPL is different from the global that you use directly on the REPL.

So check this out: I came up with what seemed like a workable hack, since we're overriding the require method anyway (to give it a sensible lookup path)...

  sandbox.require = _require = (path) ->
    # files brought in with `require` need to see `sandbox` as `global`
    trueGlobal = global
    trueGlobal.global = sandbox
    result = Module._load path, _module
    trueGlobal.global = trueGlobal
    result

...which looks crazy, but it works—kinda. It turns out that vm.runInContext works some weird voodoo, such that if you run sandbox.a in the JS passed to it, sandbox is modified; but if you run sandbox.a = 7 anywhere else while it's at work, that change gets overwritten when runInContext returns. Craziness. So, back to square one...

[michaelficarra]

Related: https://github.com/Gozala/repl-utils/blob/3a90a5b40a8ccd3b9d9d/repl-utils.js#L16-26

[TrevorBurnham]

So. I spent some time finding a way around it, but I think the best (possibly only) solution is to ditch sandbox entirely and just runInThisContext. Thoughts, Michael?

Also, the vm module (which we'd already been requiring in repl.coffee) isn't documented until 0.4, so I changed package.json accordingly—unless there's a reason why it should still be 0.2.5?

[michaelficarra]

I'm still not convinced at all that there's a reason you want a module having access to your scope. It's a "time saver"? How? Even if that was true, it violates my principle of least privilege. In the current model, I trust modules only with objects to which I give them references. I don't want to require 'coffee-script' then require 'possibly-malicious-module', and have it overwrite my compiler, pulling a Ken Thompson.

As for the node requirement, I don't think it's unreasonable at all to bump it to 0.4.0. Maybe that should be a separate pull, though.

[TrevorBurnham]

Agreed about the package.json commit; moved to #1663.

[michaelsbradleyjr]

@michaelficarra You make a good point about limiting a module's access to the requiring process's global scope.

But note that such a limitation doesn't exist when programs are run and modules are loaded with require outside the REPL, i.e. in the usual manner. This variance in behavior is pointed out in #1654, and the question was originally raised a couple of days ago in #5 for Smooth-CoffeeScript.

Even in node's REPL there is some "weirdness" with respect to the modules and globals. See my comment on #1654.

Supposing I agreed that I did not want a module to access the requiring process's global scope directly .. okay, but the globalize() functions I cooked up for "globalizing" certain module's properties post require don't work either.

Two thoughts:

(1) That require-modules-global behavior varies between running programs in the usual way vs. banging around in the REPL is confusing, at least it seems so to me.

(2) While writing modules so as to directly modify their requiring process's global scope may well be an anti-pattern, I would at least like to be able to explicitly "globalize" some properties of my modules. Now I don't do that all the time, but I may write a module the very purpose of which is to (optionally) introduce some "sugar" into the global scope.

Suppose I write a module myStuff.js and export meth1, meth2, meth3. And suppose I'm planning to use those methods over and over and over again, so I've also exported a globalize method from myStuff. Then I want to be able to run:

var myStuff = require('./myStuff')
myStuff.globalize()

Now I can make calls to meth1() instead of myStuff.meth1() and so on.

This is entirely possible when running node.js scripts in the regular way, written in both JavaScript and CoffeeScript. But globalize will not work correctly in neither node's nor CoffeeScript's REPL. And that's the point: it seems like there shouldn't be a big difference between the two ways of running scripts.

[michaelficarra]

@michaelsbradleyjr:

so I've also exported a globalize method from myStuff

I don't think that should be possible. The global you're referencing in the globalize method is not the same object as the global that is referenced in the REPL. Just define

use = (moduleName) -> global[g] = v for g, v of require moduleName

in the REPL. Or if you'd like it to be explicit, export a globals object:

use = (moduleName) -> global[g] = v for g, v of (require moduleName).globals

[TrevorBurnham]

Michael, I still don't understand your opposition.

It's a "time saver"? How?

As you well know, CoffeeScript's Cakefile exports several globals for testing, providing a nice little DSL. It's really nice to be able to just write ok condition without having {ok} = require 'assert' at the top of each of 20 files. Surely you can see how it would also be nice, if you were require-ing several things every time you open up the REPL, to just require a single file that, in turn, added the things you need to global?

I don't want to require 'coffee-script' then require 'possibly-malicious-module', and have it overwrite my compiler.

Do you really see a security issue here? I'd be much more worried about things I require being able to require 'fs' and require 'child_process'. There are ways you can safely interact with 'possibly-malicious-module' (e.g. a virtual machine), but the REPL isn't one of them.

[TrevorBurnham]

Btw, I think the current patch is pretty good, and I hope you'll take a look. It's actually provides better behavior than the Node REPL's, IMHO—the test I mentioned before, with (global.a = 23) && (require './a') and (require './a') && (global.a = 23), passes. So, this patch makes the REPL much more consistent with the behavior you get from evaluating expressions with coffee file.coffee or coffee -e "expression".

[jashkenas]

I don't think I have the requisite REPL experience to make a judgement call about this one (I rarely touch the thing). @michaelficarra -- if you're comfortable with the final state of this patch, do you mind reviewing/merging it?

In general, I think we should strive for the behavior of the Node.js REPL for issues like this one.

[michaelsbradleyjr]

I just want to note, in a brief way, what I think is the best argument in favor of Trevor's patch:

It allows for strong symmetry between how one may manipulate the global environment when running scripts in the normal way and when one is experimenting, developing, learning, debugging in the REPL.

Without Trevor's patch, there is an asymmetry between the two ways of running scripts, with respect to manipulating global scope.

If Trevor's patch has no implications beyond repairing the asymmetry, then why not accept the patch?

If there are implications, what are they?

I've been using Trevor's fork, with the globals-REPL patch, and have not yet experienced any drawbacks; on the contrary, I find his patch beneficial.

That's my two cents.

[autotelicum]

+1 for consistency.

I have often found it annoying, when browsers (firefox and chrome with canvas/getImageData) or operating systems (windows) restrict what I can do from a local file - that I just wrote - when no such restriction is imposed on files coming from a web server - and as with this issue if there was to be a difference then it should be the other way round - allowing me to do quick-n-dirty things in the REPL that I can not do in an npm module.

When 1.1.2 came out I ran all my code/tests as programs and they all passed. It was surprising to me when it was reported that the code did not work with -r and the REPL. It is a burden having to repeat all the tests manually in the REPL.

Remember that it is terribly off-putting when you grab a tutorial and a programming environment, follow the instructions and then it does not work. To minimize the chance of that happening I didn't refer to npm but instead included copies of underscore, coffeekup, qc, etc. I assumed that I would notice changes in CoffeeScript as they happen.

I have performed some tests without any issues such as the bash/grep like session I showed in issue #1654. But honestly not a whole lot. I usually work in 2D (in my editor/execution environment) rather than the 1D line oriented REPL.

Concerning the bigger question about globals. I can understand if such a restriction is put on modules that come from npm. I think that would be something that requires the cooperation of the node project. BTW Node has a good way of deprecating things, printing: "Use process.env instead of process.ENV". If global is to be retired then issuing a warning in the preceding version is a very user-friendly way to do it.

[michaelficarra]

I do not agree with the changes to /src/repl.coffee. It's just not appropriate to give the scripts being run in the REPL access to the context object of the sandboxing script. The way we're generating a sandbox object right now is just fine; the proposed use case of actually wanting to give required scripts (so, different modules) access to the same global object is not even close to convincing. The alternative method I proposed in #1654 -- actually taking advantage of the module system provided by node -- is a much better solution than this, especially with how easily and cleanly the desired result can be had using the function I noted above.

-1

[autotelicum]

Changelog node 0.5.10 (https://github.com/joyent/node/wiki/ChangeLog)

...
#1484, #1834, #1482, #771 Don't use a separate context for the repl. (isaacs)
...

See nodejs/node-v0.x-archive#1904

[TrevorBurnham]

Thanks for that, autotelicum. As jashkenas has said,

In general, I think we should strive for the behavior of the Node.js REPL

On that basis alone, this change would seem to be justified.

But I'd still like to understand what you're proposing, Michael. You maintain that it's preferable for global to be a different object in modules required from the REPL than in the REPL proper. In my mind, that seems contrary to what global is for in Node: sharing state across the entire process. Why should the REPL process behave in a more strictly modular manner than an ordinary Node process?

[michaelficarra]

@TrevorBurnham: I just don't like how my require and my Array and my Object can all be modified by every module I require. But since nodejs/node-v0.x-archive#1904 was merged, I guess it would only be more confusing to behave differently than them. If @jashkenas is still comfortable with it, and you update the pull to master, I'll approve this.

[michaelsbradleyjr]

I just recently experienced some interesting behavior with respect to Trevor's patch and instanceof, along the lines of what you're saying with regard to the built-in prototypes: Object, Array, etc.

I'm going to experiment with the REPL in node v0.4.12 vs v0.5.10, and compare to coffee's normal REPL vs. Trevor's patch, and will report back shortly.

It may be that the coffee REPL should follow more closely the direction taken in node's REPL v0.5.10 rather than adoption of Trevor's patch.

[TrevorBurnham]

@michaelsbradleyjr OK, I'll hold off while you look into that. Thanks.

[michaelsbradleyjr]

Summary conclusion: Nevermind, Trevor's patched REPL and the REPL for node v0.5.10 are producing identical results. But this is what I was testing...

Consider the following two scripts:

b.coffee

exports.arr = []

a.coffee

b = require './b'

console.log (b.arr instanceof Array)

exports.arr = b.arr

In the same directory I ran compile -bc *.coffee

Here's the output from my REPL sessions...
(NOTE: coffee REPL v1.1.2 seems broken running in node v0.5.10)

node REPL v0.4.12

-> % node
> var a = require('./a')
true
> a.arr instanceof Array
false

node REPL v0.5.10

-> % node
> var a = require('./a')
true
> a.arr instanceof Array
true

coffee REPL v1.1.2 (node v0.4.12)

-> % coffee
coffee> a = require './a'
true
{ arr: [] }
coffee> a.arr instanceof Array
false

coffee REPL - Trevor's patch (node v0.4.12)

-> % coffee
coffee> a = require './a'
true
{ arr: [] }
coffee> a.arr instanceof Array
true

Running a.js / a.coffee directly from the command line with node / coffee, for both node v0.4.12 and v0.5.10, and both coffee v1.1.2 and Trevor's patched branch, produces the same output, i.e. true is printed in the terminal. That's not unexpected, but just indicating it for completeness.

[TrevorBurnham]

Cool. Thanks for that, @michaelsbradleyjr. We should definitely get on top of Node 0.5.x compatibility soon.

[jwarchol]

I apologize for commenting on this old, closed issue, but has this been reverted(?) as of 1.5.0? I noticed in 1.5.0 that the CoffeeScript repl now handles global properties differently than the node reply (0.8.19 in my case). Before I opened a new issue, I had hoped @TrevorBurnham could chime in very quickly to let me know if there was something key I was missing about the intended current state of global in the CoffeeScript repl.