Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node: Make builtin libs available for --eval #6207

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

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.

@mscdex mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 15, 2016
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 15, 2016
@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

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

@@ -101,10 +101,14 @@
delete process.env.NODE_UNIQUE_ID;
}

var internalModule = NativeModule.require('internal/module');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

exports.builtinLibs = ['assert', 'buffer', 'child_process', 'cluster',
'crypto', 'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'https', 'net',
'os', 'path', 'punycode', 'querystring', 'readline', 'stream',
'string_decoder', 'tls', 'tty', 'url', 'util', 'v8', 'vm', 'zlib'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Object.defineProperty(object, name, {
get: () => {
const lib = require(name);
object[name] = lib;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

delete object[name];
object[name] = val;
},
configurable: true
Copy link
Member

@bnoordhuis bnoordhuis Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bnoordhuis bnoordhuis Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@addaleax addaleax added the repl Issues and PRs related to the REPL subsystem. label Apr 18, 2016
@evanlucas
Copy link
Contributor

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.

As part of this, the list of builtin modules and the code for
adding the corresponding properties to the target context is moved
to `internal/module.js`, and the previously missing `repl` entry
is added.
@addaleax
Copy link
Member Author

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/

Make sure that the built-in modules in the repl stay non-enumerable.
Previously, they would pop up as enumerable properties of the global
object after having been accessed for the first time.
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

LGTM

@bnoordhuis
Copy link
Member

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

@addaleax
Copy link
Member Author

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

@cjihrig
Copy link
Contributor

cjihrig commented Apr 18, 2016

LGTM. CI is green.

jasnell pushed a commit that referenced this pull request Apr 18, 2016
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.

As part of this, the list of builtin modules and the code for
adding the corresponding properties to the target context is moved
to `internal/module.js`, and the previously missing `repl` entry
is added.

PR-URL: #6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 18, 2016
Make sure that the built-in modules in the repl stay non-enumerable.
Previously, they would pop up as enumerable properties of the global
object after having been accessed for the first time.

PR-URL: #6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

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

MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Make sure that the built-in modules in the repl stay non-enumerable.
Previously, they would pop up as enumerable properties of the global
object after having been accessed for the first time.

PR-URL: #6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069
This was referenced Apr 21, 2016
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
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.

As part of this, the list of builtin modules and the code for
adding the corresponding properties to the target context is moved
to `internal/module.js`, and the previously missing `repl` entry
is added.

PR-URL: #6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
Make sure that the built-in modules in the repl stay non-enumerable.
Previously, they would pop up as enumerable properties of the global
object after having been accessed for the first time.

PR-URL: #6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behavior was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
@cpojer
Copy link

cpojer commented Apr 22, 2016

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

@addaleax
Copy link
Member Author

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

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

cpojer commented Apr 25, 2016

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

@addaleax
Copy link
Member Author

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

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
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.

As part of this, the list of builtin modules and the code for
adding the corresponding properties to the target context is moved
to `internal/module.js`, and the previously missing `repl` entry
is added.

PR-URL: nodejs#6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
Make sure that the built-in modules in the repl stay non-enumerable.
Previously, they would pop up as enumerable properties of the global
object after having been accessed for the first time.

PR-URL: nodejs#6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) nodejs#5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) nodejs#6279
  * update ESLint to 2.7.0
    (silverwind) nodejs#6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) nodejs#6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) nodejs#6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) nodejs#6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) nodejs#6090

src:
  * add SIGINFO to supported signals
    (James Reggio) nodejs#6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) nodejs#6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) nodejs#6069

PR-URL: nodejs#6322
jasnell pushed a commit that referenced this pull request Apr 26, 2016
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.

As part of this, the list of builtin modules and the code for
adding the corresponding properties to the target context is moved
to `internal/module.js`, and the previously missing `repl` entry
is added.

PR-URL: #6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Make sure that the built-in modules in the repl stay non-enumerable.
Previously, they would pop up as enumerable properties of the global
object after having been accessed for the first time.

PR-URL: #6207
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants