node: Make builtin libs available for --eval

[addaleax]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

cli

Description of change

Make the builtin libraries available for the --eval and --print CLI options, using the same mechanism that the REPL uses.

This renders workarounds like node -e 'require("fs").doStuff()' unnecessary which I find myself often using.

[jasnell]

oh, yes please. Haven't done a complete review yet but would love for this to happen.

[bnoordhuis]

Can you move this into the branches of the if statement below? It avoids the hit of loading the module unless it's actually used.

[Fishrock123]

Hmmm, I feel like there was an issue for this, but search doesn't return anything.

[Fishrock123]

Another good excuse for #3307?

I guess it doesn't matter too much. The only thing that is remotely on the roadmap as a module is async_wrap.

[addaleax]

I guess it doesn't matter too much.

Agreed. And I just noticed that the list misses the repl module itself? Should I add that myself here? 😄

[jasnell]

+1 for having an API that exposes the list. Let's be sure to make it read-only so we don't end up with another process.config situation ;-)

[Fishrock123]

CI: https://ci.nodejs.org/job/node-test-pull-request/2276/

Seems good to me, I'm unsure if more tests are necessary or not, but the repl test doesn't seem much more comprehensive either: https://github.com/nodejs/node/blob/master/test/parallel/test-repl-autolibs.js

[thefourtheye]

Isn't this overriding the value set by set below?

[addaleax]

@thefourtheye Nah, it’s a nice but weird trick. The setter below deletes the property before re-assigning it, which disables the setter/getter themselves. So only the first assignment or read of this variable uses this mechanism, and nothing gets overridden.
I was a bit confused at first, too, so I guess it’s a good idea to add a comment for explanation. 😄

(This code has essentially only been moved from lib/repl.js, btw)

[addaleax]

Updated again with more explanatory comments + added repl to the list of builtin modules.

[bnoordhuis]

Maybe also set enumerable: true? The delete+assign setter trick is alright but it makes the property enumerable.

EDIT: I guess that could have unexpected consequences. Perhaps just make sure it stays non-enumerable.

[addaleax]

Perhaps just make sure it stays non-enumerable.

@bnoordhuis That’s probably doable, but I think it would get a lot trickier to maintain the current behaviour of allowing to re-assign these globals. Correct me if I’m wrong, but to do this, you’d have to:

• Stop invoking the setter from the getter, and
• Call delete from there “manually”, and use Object.defineProperty({ value: … }) to set the reference to the module, and
• Give that new property descriptor the setter that is used right now, too, so that in the case of user-defined globals with these names they are enumerable as expected

Maybe there’s a simpler way, and I agree that it’s a bit weird the way it is right now (and has been for the last three years), but I don’t think anybody really cares very deeply.

[bnoordhuis]

Wouldn't this work?

function get() {
  const value = require(name);
  Object.defineProperty(object, name, { value, writable: true });
  return value; 
}
function set(val) {
  void object[name];  // Property access for side-effect.
  object[name] = val;
}
Object.defineProperty(object, name, { get, set, configurable: true });

[addaleax]

It would leave the property non-enumerable in all cases, even when manually overwritten by the user. If that’s okay, sure, that basically works (It has to be delete object[name] where you wrote void object[name] though, so that the setter does not enter infinite recursion).

[addaleax]

@bnoordhuis Updated this with the behaviour I think you had in mind

[evanlucas]

CI again: https://ci.nodejs.org/job/node-test-pull-request/2296/

[addaleax]

Rebased & squashed into two commits that I’d consider rather orthogonal changes, PTAL :)
Also new CI because the previous one failed, probably unrelated, but it might be a good opportunity to try running that myself for the first time: https://ci.nodejs.org/job/node-test-pull-request/2297/

[jasnell]

LGTM

[bnoordhuis]

LGTM2. Traditionally, the first line of the commit log is all lowercase.

[addaleax]

@bnoordhuis Yeah, I already noticed that I screwed that up again… someday I’ll get it all right. 😄

[cjihrig]

LGTM. CI is green.

[jasnell]

Landed in 39d905e and ebc8c37. Fixed the commit log nits on landing. Thanks!

[cpojer]

fyi this change breaks code like node -e "const path = require('path');" which might be found in existing scripts.

[addaleax]

@cpojer I think that went unnoticed by me because it doesn’t happen with the current master or the current Node.js v6 release candidates…

[addaleax]

@cpojer Also, the --eval/--print argument is executed in sloppy mode, not strict mode. In Node.js v5 that means that const doesn’t work as expected anyway.

[cpojer]

It would probably be nice to mention this behavior change somewhere.

[addaleax]

@cpojer Got any suggestions for that (esp. the “somewhere”)? The docs have been amended to reflect the fact that these modules are now predefined, and it’s listed in the changelog as a notable change.