Don't use separate context in repl

[isaacs]

Running the repl code in a separate process is a giant pain that only serves to confuse users.

Suggestion:

1. Change the vm.runInContext with vm.runInThisContext
2. Remove repl.context (or replace with a link to global?)
3. Remove .clear function (does anyone actually use this anyway?)

[sethml]

#1482
#771

[bnoordhuis]

Remove repl.context (or replace with a link to global?)

Wouldn't that be a regression of #232?

Remove .clear function (does anyone actually use this anyway?)

I didn't even know it existed. :-)

[isaacs]

Wouldn't that be a regression of #232?

No, because global.global === global already. That issue was about making repl.context.global === global.

[eventualbuddha]

#1834, while we're listing "confusion" bugs.

[ELLIOTTCABLE]

-1, personally. This is just going to reinforce “bad assumptions” that many are already making about JavaScript. This sort of half-arsedness might be fine in the browser, where “normal development” only leaves you dealing with a single v8::Context-style environment, but that’s not the case on the server.

Instead of trying to “fix” this, why don’t we try and educate the users? Perhaps a section in the documentation (yes, it’s a language issue, not a Node issue, but it still widely affects our users, and is something they’ll come into more contact with Node than they would have in previous interactions with JavaScript), and then include a warning with a link to that subsection under the REPL section, the VM section, the modules section, etc? Describe the concept that Object in one context is not the same as Object in another context, and thus instanceof and friends are a bad idea.

Again, ‘feels’ outside the scope of Node, but might be a necessity for user-experience purposes: a set of convenience functions for comparing “types” (constructors.) cross-context, something at a higher ‘outside’ level from the user’s module code or the REPL, possibly even exploiting v8 APIs to avoid all sorts of cross-checks (I haven’t yet looked, does v8 expose something like that, to compare the original Context-provided global-proxy members against a given Function? We could probably put something together.)

[eventualbuddha]

I think you're right, @ELLIOTTCABLE, that this might be outside the scope of node. I've personally never liked that Array, Object, etc were all different in different contexts. It makes the already weak type tools in JS that much weaker. That said, I understand that's how it is right now and I'm not sure what the right way to deal with it is.

[sethml]

elliottcable: Unless you're proposing changing the default behavior of require() in node, I don't think your argument makes sense. Right now, the REPL and code run directly in node behave very differently. The REPL is predominantly used for testing/debugging, so making code behave radically differently while debugging than in production is a recipe for confusing users to no good end.

Your argument could be applied to encourage changing the default behavior of require() to create each module in a new context (and I can definitely see arguments for doing so), but good luck with that: you'd break the functionality of a large number of existing modules.

[isaacs]

@sethml You can make node load modules in separate contexts by setting NODE_MODULE_CONTEXTS=1, but no one actually does this. It's slow and terrible, and makes for weird behavior.

[ELLIOTTCABLE]

@sethml, what @isaacs said;

@isaacs, I was under the impression that we were moving towards flipping that on as the default. “Slow?” It’s not remotely slow, or shouldn’t be unless Node’s internals are doing something fairly wrong (at least, so it seems from my knowledge of v8); with snapshots, creating new v8::Contexts is trivially performant. Unless you’re talking about “slow and terrible” in some other sense …

Anyway, besides possible slow-and-terrible-ness that I’m missing, I certainly understand the ideological arguments for/against it. I’m sure it’s something that’s had a lot of thought put into it, and will have much more yet. For whatever reason, I’d just gotten the impression that the general trend was towards eventually flipping that Big Lever to turn it on by default.

Long story short: I think the default behaviour of the REPL and the, er, “module environment” should definitely mirror eachother. In fact, the behaviour of the REPL module should almost certainly mirror the state of the NODE_MODULE_CONTEXTS flag.

[sethml]

Yeah, I'm aware of the NODE_MODULE_CONTEXTS=1 environment variable. I strongly agree that the behavior of the REPL and normal code should match.

[ghost]

-1. Nothing will then force you to make module system work right with contexts and to fix vm.runInThisContext bug when called from other than main context (#898). Lots of problems may disappear if those are fixed (they are related, of course, module system is strange in multiple context because of #898, among others). Better fix the cause, not the symptom.

[isaacs]

@Herby Bugs in vm.runInThisContext need to be fixed regardless. Causing needless confusion with the repl is harmful.

[isaacs]

@ELLIOTTCABLE I have seen no indication of making NODE_MODULE_CONTEXTS the default any time soon. It's too confusing, for too little benefit.

[ELLIOTTCABLE]

@isaacs I think @Herby is basically suggesting we shelve the REPL issues until the Context/module issues are solved, so we can then evaluate the state of these issues at that time as opposed to their current state. The issues and/or the requirements of the situation may have changed, then.

[isaacs]

@ELLIOTTCABLE Yeah, I know. But that's not the proper priority.

The repl issues are much more damaging and confusing, especially to new users who are most susceptible to being confused by node, than the vm bugs. Either we change the module loading default now, or we make the repl not use vm.runInNewContext.

The simplest fix, which I really can find no harm in, is to just not make the repl run in new contexts. We can always put the context wrapping back in the repl once the vm bugs are fixed, and the module loading default is changed, but really, I don't see that happening any time soon, and it should not delay this fix, which is easy and simple, and will reduce confusion.

[ghost]

@isaacs: Well, if it is meant to be temporary, and somehow flagged as such, ok. I just see so many temporary things becoming permanent in this world... :-/
Because I see running REPL in different context as good thing. I'd use context more pervasively, you probably know. ;-)

[ELLIOTTCABLE]

@isaacs I think you misunderstood my point. I think neither change should happen, as a specific; instead, I think, the change should be making the REPL implementation use NODE_MODULE_CONTEXTS setting, so it’s always directly in line with the “rest of the environment.”

That would solve this issue and make things more intuitive. That also addresses @Herby’s and my concern about fresh contexts not being pervasive enough: we can go ahead and use NODE_MODULE_CONTEXTS ourselves, and advocate its use, without changing REPL such that it “hurts” those who disagree.

[isaacs]

@ELLIOTTCABLE I'm not opposed to making it respond to the env flag. Looks like @ry LGTM'd the patch, so it's going in.

[eventualbuddha]

@isaacs: Awesome, thanks!