Cannot extend global prototypes in the REPL

[TooTallNate]

See this gist: https://gist.github.com/865250

When running a regular node script via node script.js, other modules can extend the global prototype's of the built-in JavaScript classes (Object, String, etc.). But when trying to load a module from the REPL that does the same, the global prototypes remain untouched.

This isn't a huge deal as the REPL is only used for testing, and the expected functionality works when running a script normally. But it would be nice to be able to test the extensions through the REPL as well...

[isaacs]

Extending globals won't work when NODE_MODULE_CONTEXTS is turned on,
and it's considered by many to be bad form.

What you're describing is an intentional feature.

On Thu, Mar 10, 2011 at 16:31, TooTallNate
[email protected]
wrote:

See this gist: https://gist.github.com/865250

When running a regular node script via node script.js, other modules can extend the global prototype's of the built-in JavaScript classes (Object, String, etc.). But when trying to load a module from the REPL that does the same, the global prototypes remain untouched.

This isn't a huge deal as the REPL is only used for testing, and the expected functionality works when running a script normally. But it would be nice to be able to test the extensions through the REPL as well...

#771

[TooTallNate]

Ok sure, then just to clarify, NODE_MODULE_CONTEXTS is turned on by default in the REPL, but turned off by default when executing a node script? Because that's the behavior I'm experiencing...

In any case, I'll just export a modified version of the Date object in my lib, thanks anyways.

[isaacs]

Yeah. That's how .clear is implemented.

On Thu, Mar 10, 2011 at 21:43, TooTallNate
[email protected]
wrote:

Ok sure, then just to clarify, NODE_MODULE_CONTEXTS is turned on by default in the REPL, but turned off by default when executing a node script? Because that's the behavior I'm experiencing...

In any case, I'll just export a modified version of the Date object in my lib, thanks anyways.

#771 (comment)

[aseemk]

I also ran into this and would like to re-request it.

I can understand the reasoning (thanks for providing it!), but it would be nice to have consistent behavior between the shell and files. Just a basic tenant of good software I guess -- it shouldn't surprise you or behave in unexpected ways.

Perhaps that means that modules in Node just should also be in their own context, without the ability to mess with globals. That would eliminate some valuable use cases though, like TJ's should.js.

So here's a strawman:

1. Keep support for modifying globals. It can indeed be used for evil, but it can also be used for good.
2. Drop support for .clear by default in the shell; have the shell execute modules in the same context like files.
3. Add a config or optional param to node to execute modules in a new context. This would apply to both the shell and files.
4. Have .clear output a warning if called from a shell without that config or param.

Thanks for your consideration!

[dresende]

What if you create a test.js that loads the REPL. Doesn't work for testing?

[isaacs]

In my opinion, we should just make NODE_MODULE_CONTEXTS the default.

Relying on shared access to the same global context is an antipattern that browsers foisted on us. We have better tools now.

[isaacs]

Add a config or optional param to node to execute modules in a new context. This would apply to both the shell and files.

NODE_MODULE_CONTEXTS=1 node program.js

[awwright]

That would cause other unintended effects, consider:

Date==require('./b').Date;

This evaluates true with NODE_MODULE_CONTEXTS=0, and false with NODE_MODULE_CONTEXTS=1... Of course you can't be talking the same object if each of them have different methods.

Exactly the effects that such isolation would have isn't quite intuitive:
require('./b').arr instanceof Array; // false
Array.isArray(require('./b')); // true
require('./b').string.constructor == String; // true
require('./b').stringConstructor == String; // false

[Floby]

When creating several contexts, V8 rebuilds the default objects (Object, Array, Date, Math, etc.). Even if what you describe is unintuitive, this is what contexts mean.
see this http://code.google.com/apis/v8/embed.html#contexts

[ghost]

@isaacs "In my opinion, we should just make NODE_MODULE_CONTEXTS the default."

This will never get through. Performance. I mean, not performance, every module is loaded once and then cache-served, but startup time.

@isaacs "Relying on shared access to the same global context is an antipattern that browsers foisted on us. We have better tools now."

I'd disagree. (and, what did you exactly mean by "better tools"?)
Possibility to enhance existing classes is important part of dynamic languages from the time of Smalltalk. There it works, and is often used, and not so frowned upon, technique (some of it, like "isInteger" etc. in Object are due to architecture of the language, in JS this would not be needed in Object, but would be in Integer (that is, Number, here) anyway),
In any case, this makes true use of OO polymorphism. If you prohibit extending base classes (either by design as in Java and before, or by convention, you make the language lot less powerful, I'd say crippled.

I would say what you blame the browsers is not their fault, this is written entirely in the foundations of JS as a langauge - it is mix of functional {heavy use of closures) and OO. This means, you should be able to use polymorphism to its full extent inside the code you pass around in closures - and this is the true point where you need to be able to augment Object, Number, Array and String. It's true that some things can be done in pure "functional" way by calling the function on an object, but there are lots of things that OO and its polymorphism is the right tool to use (and this may be really personal or philosophical, but I was taught that polymorphism should be used heavily, since it is clear way to solve "do this differently based on object type", which is far too common (lots of switch statements could be rewritten with good results for the readablity and maintainability to use polymorphism instead; not all, of course)).
Closures hold their lexical scope, so they know their variables and functions; but they do not carry with themselves the OO bindings of their environment; that is I think the main cause of the problem when passed between contexts (and the multiple-root thing where instanceof Object fails).
Sorta humorous is, that this only holds for the base classes - if you use your own classes, defined in a module that is imported multiple times, you can augment it and it happily works everywhere (since there is only one copy of it, even in multiple context environment).

[ghost]

To sum my subsequent lines of throught on this matter:

1. Within the application, there should be no contexts used for separating modules. Within one app, every app classes are shared, and so also Object, Number etc. should be shared. The "context" is anyway more appropriate for an app, not for every single module that it is built from.
   If you use contexts in your app, you can, but you have been warned.
2. If there are more "apps" to be run, they should be separated completely - nothing shared. So not only should they be in different contexts, but also modules they require should be different (of course, so they use the actual Object, etc.). This includes even the native modules.
   2a. One of the possibilities to separate apps is to launch child process.
   2b. Other possibility would be to allow to create a "jail" within node, where not only new context is started, but where the basics (native modules, moduel system, module cache, etc.) are reinitialized for that jail. Then, main script should run in such a jail, repl should run in separate jail of its own (but within it, everything's shared, of course), and the main app, if it is just some sort of multiplexer, should have possibility to create other jails to run separate applications within them.
   The "recompile for every jail" is not the performance problem - a Script can be created and cached for each module in some sort of master cache, then "compiling" the module in new jail in just the matter of running this procompiled Script in the context of the jail (this was my motivation for having unbound Scripts from the beginning), If you like the microkernel view, this master cache can run as an app of its own, in the jail of its own (some metacircularity that arises could surely be somehow solved).

[aseemk]

@ry, @isaacs et al.:

Have you guys ever considered support for optional params/args to pass when require()-ing modules? This would let you change the default but have clients explicitly pass in the global context (or objects/prototypes within it) if they want the module to change it.

// assuming NODE_MODULE_CONTEXTS is on by default
var foo = require('foo');    // guaranteed to not mess with my global context
var foo = require('foo', Array.prototype);   // giving foo access to my context's Array prototype; saying i trust it to add to it

This would be an awesome feature IMHO! A safe default with explicit and elegant opt-in. And it would solve this issue as well. What are your thoughts?

[aseemk]

Btw, I just realized something weird: if modules run in a different context when require()-ed from the shell, how is my module able to monkey-patch console.dir?

$ node
> console.dir(Function.prototype)
[Function: Empty]
> require('dir')
[Function: dir]
> console.dir(Function.prototype)
// a bunch of output, omitted

Is there something special about the console object?

[tlrobinson]

@aseemk The require() API is a pseudo-standard implemented by more than just Node.js with the goal of being able to share modules between platforms, so it's is not recommended that platforms extend it.

Furthermore, this specific extension doesn't make much sense because require() is expected to execute only once and always return the same object. Given those semantics, what would this mean:

require('foo', Array.prototype);
require('foo', String.prototype);

I think a better solution for this specific case is you export a function that will modify the object and you explicitly ask it to, e.x.

var foo = require('foo')
foo.augment(Array.prototype);
foo.augment(String.prototype);

[aseemk]

@tlrobinson Great point! Thanks for the reminder.

I had been thinking of the same idea, but generalized, e.g. requireInto() or pollute() as a wrapper around require(). I love your augment() suggestion as something that modules can support if they want:

require('should').augment(Object)

Thanks!

[isaacs]

This whole thing seems like such a non-issue. Repls are very rarely perfect 1:1 replicas of the system they represent. (Have you ever really looked hard at the Webkit console or Firebug environments? They're insane.) Even bash is significantly different in interactive mode than it is from inside a script.

Having .clear to reset your session is really handy. If you're really running into incompatibility issues in the repl, then maybe that should be a sign that it's time to fire up your text editor. Our repl is surprisingly polished and humane for such a young project.

[aseemk]

Fair points! It's indeed a minor issue, so no big deal. Thanks for your consideration. =)

[ghost]

Taken literally, it is a non-issue.
Taken from higher perspective, by leaving such non-issues be, you can make your system more vulnerable and brittle, and by solving them, you can get a cleaner solution.

[bminer]

I ran into this issue, as well. Weird. If I run 'node foo.js', it prints true. If I run 'node', the the contents of foo.js verbatim, I get false (even if I manually do NODE_MODULE_CONTEXTS=0). Come on... NODE_MODULE_CONTEXTS=0 should work, right?

...foo.js
var d = new Date();
var x = require('./bar');
x(d);

...bar.js
module.exports = function(ts) {
console.log(ts instanceof Date);
}

[isaacs]

I think maybe 5 "wtf" bugs is enough. Reopening this, so that it can be yet another issue closed by the commit that fixes #1484.

[bminer]

Thanks, @isaacs. Yeah, using a separate context for the REPL is an interesting idea, but when people are testing their modules using the REPL, it adds an extra layer of confusion. Anyway, thanks for taking the time to put a pull request together.

[still-dreaming-1]

If I understand this correctly the REPL no longer uses a different context? My monkey patch methods are still not recognized in the REPL? is that intentional?

[addaleax]

@still-dreaming-1 Hi! This is a pretty old issue, basically all new discussion happens over at https://github.com/nodejs/node.

That being said, nodejs/node#5703 which was released in Node v6.3.0 changed it to using a different context, but that unfortunately had some unintended side effects (x x x x) so it ended up getting reverted in v6.3.1.

My monkey patch methods are still not recognized in the REPL?

You’ll probably have to be more specific – the fact the REPL does use the same context should allow you to monkey-patch JS globals, not the other way around.