Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Cannot extend global prototypes in the REPL #771

Closed
TooTallNate opened this issue Mar 11, 2011 · 23 comments
Closed

Cannot extend global prototypes in the REPL #771

TooTallNate opened this issue Mar 11, 2011 · 23 comments

Comments

@TooTallNate
Copy link

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
Copy link

isaacs commented Mar 11, 2011

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
reply@reply.github.com
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
Copy link
Author

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
Copy link

isaacs commented Mar 11, 2011

Yeah. That's how .clear is implemented.

On Thu, Mar 10, 2011 at 21:43, TooTallNate
reply@reply.github.com
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
Copy link

aseemk commented Apr 8, 2011

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
Copy link

dresende commented Apr 8, 2011

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

@isaacs
Copy link

isaacs commented Apr 8, 2011

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
Copy link

isaacs commented Apr 8, 2011

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
Copy link

awwright commented Apr 9, 2011

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
Copy link

Floby commented Apr 9, 2011

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
Copy link

ghost commented Apr 9, 2011

@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
Copy link

ghost commented Apr 9, 2011

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
Copy link

aseemk commented Apr 9, 2011

@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
Copy link

aseemk commented Apr 9, 2011

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
Copy link

@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
Copy link

aseemk commented Apr 9, 2011

@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
Copy link

isaacs commented Apr 9, 2011

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
Copy link

aseemk commented Apr 10, 2011

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

@ghost
Copy link

ghost commented Apr 10, 2011

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
Copy link

bminer commented Oct 6, 2011

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 isaacs reopened this Oct 18, 2011
@isaacs
Copy link

isaacs commented Oct 18, 2011

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.

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Oct 18, 2011
Fix nodejs#1484
Fix nodejs#1834
Fix nodejs#1482
Fix nodejs#771

It's been a while now, and we've seen how this separate context thing
works.  It constantly confuses people, and no one actually uses '.clear'
anyway, so the benefit of that feature does not justify the constant
WTFery.

This makes repl.context actually be a getter that returns the global
object, and prints a deprecation warning.  The '.clear' command is gone,
and will report that it's an invalid repl keyword.  Tests updated to
allow the require, module, and exports globals, which are still
available in the repl just like they were before, by making them global.
@bminer
Copy link

bminer commented Oct 18, 2011

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.

@isaacs isaacs closed this as completed in b70fed4 Oct 19, 2011
@still-dreaming-1
Copy link

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
Copy link
Member

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants