adding npm as a default REPL command #2974

Closed
wants to merge 1 commit into
from

4 participants

@seebees

This is in response to #2931. Crazy Talk ACCEPTED!

I still need to write tests of course, and stream the input for a beter use experience, but as a proof of concept?

If I adjust these lines then you could even handel npm not just .npm

In any event I would use the REPLKeyword engin to solve this problem.

My, I even love how the prompt does not come back until npm returns! It's the small things...

@TooTallNate

Well the fact that users need to type in .npm needs to be fixed, otherwise it's missing the point. But I don't think modifying the REPLKeyword structure is the right way to do that.

@isaacs and I were talking about using a custom evaluator for the global node repl. I don't think this npm logic should be part of any other REPL.

@Benvie

Allowing arbitrary commands has worked fine in my usage requisite on a few things. First, the keywords are obvious and the first thing shown every time the repl loads is the command to see the keybinds. Showing the keywords/keybinds. And third making them pretty easy to change. And four I guess, keywords must start the line.

@seebees

@TooTallNate sadness I can make it do that, if you like. I kind of liked the idea of being able to type help into the REPL, which is why I suggested it.

In any event if I change this to use a custom evaluator to look for just npm would it be OK if I wire it to actually run the command? Because it seems simple to me now. I figure that if I do all that and it becomes crazy in windows or something, I can just fall back to outputting a message and we are all still buck's up.

@Benvie

F1 would make a pretty good keybind for showing help ;)

@TooTallNate

@seebees Actually, I am open to the idea of changing the behavior of keywords, if there's good enough reason to. But I think it belongs in a separate issue/pull request, so that others can chime in and it'll be on-topic. If changing the behavior of keywords comes first then this pull would be ready to go potentially.

@Benvie Keybindings are a different topic than keywords. Sounds like territory for another issue/pull request ;)

@TooTallNate TooTallNate commented on an outdated diff Mar 21, 2012
lib/repl.js
@@ -822,6 +822,25 @@ function defineDefaultCommands(repl) {
this.displayPrompt();
}
});
+
+ repl.defineCommand('npm', {
+ help: 'Execute npm from the REPL session',
+ action: function(cmd) {
+ try {
+ var exec = require('child_process').exec
+ , that = this;
+ exec('npm ' + cmd
+ , {cwd: process.cwd()}
+ , function(code, stdout, stderr) {
+ that.outputStream.write(stdout);
+ that.displayPrompt();
+ });
+ } catch (e) {
@TooTallNate
TooTallNate added a line comment Mar 21, 2012

exec() is async, so try/catch isn't gonna work here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seebees seebees commented on an outdated diff Mar 21, 2012
lib/repl.js
@@ -822,6 +822,25 @@ function defineDefaultCommands(repl) {
this.displayPrompt();
}
});
+
+ repl.defineCommand('npm', {
+ help: 'Execute npm from the REPL session',
+ action: function(cmd) {
+ try {
+ var exec = require('child_process').exec
+ , that = this;
+ exec('npm ' + cmd
+ , {cwd: process.cwd()}
+ , function(code, stdout, stderr) {
+ that.outputStream.write(stdout);
+ that.displayPrompt();
+ });
+ } catch (e) {
+ this.outputStream.write('Failed to load:' + file + '\n');
@seebees
seebees added a line comment Mar 21, 2012

no doubt, and if it did it would throw the wrong message ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seebees

@TooTallNate Cool man, thanks. The number of thing wrong with my code as it stands now is legion ;) Really, my point was @isaacs's "Crazy Talk" comment may not be so crazy.

I think that all this boils down to two questions

  1. Is it a good concept to execute npm in the global REPL?
  2. Is it practical to execute npm in the global REPL?

Given that 1 is true what then are the requirement?

  1. npm should only be called from the global REPL. i.e. the one you get when you start node
  2. npm must be adequate to execute npm, .npm is not acceptable.
  3. the REPL command prompt must not be displayed until npm completes
  4. npm output should be streamed to the REPL to minimize lag and provide a pleasant experience
  5. if npm is not installed or not in PATH, then I should gracefully explain to the user
  6. it should all just work on Windows without any hacks to the install package or other craziness

If I make these requirements work, then turning this pull into just displaying a message is trivial

I'll make a different pull for the help vs .help thing and see where that goes. If people like it, the I suppose the npm command logic could be added to only the global REPL? I'm not sure how the node dumps you in the REPL, so that may be a terrible idea. Or it could be added in exports.start?

again, @TooTallNate, thanks for the feedback.

@seebees

Given the above requirements I am thinking that if #2980 is merged, then the implementation here could be moved to here This would satisfy the requirements nicely I think.

@josher19

+1 on this.

Common to have previously installed a module but not have it on the NODE_PATH, so require fails.
Would be nice to be able to npm install this from node rather than have to deal with ^Z tty issues (#3295)
or open a new shell and change directories.

For me, more common to make the npm mistake on the command line rather than inside the REPL.
If possible, you should also make "node install ..." an alias for "npm install ..." from the command line,
or at least spit a nicer error message, such as:

    console.warn("Please use npm (node package manager)!\nnpm install " + process.argv.slice(2).join(" "));
% node install missing_module -g
Please use npm (node package manager)!
npm install missing_module -g
%
@TooTallNate

Closing since node v0.8.0 implemented this in a different way.

@TooTallNate TooTallNate closed this Jul 1, 2012
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Sep 21, 2015
@misterdjules misterdjules deps: backport 357e6b9 from V8's upstream
Backport 357e6b99ee3927cc075dd8d27c99b89d858f9dd5 from V8's upstream.

Original commit message:

  Add ScopeInfo constants to post-mortem metadata

  mdb_v8, a post-mortem debugging tool for Node.js, allows users to
  inspect ScopeInfo structures in order to get more information about
  closures.

  Currently, it hardcodes the metadata it uses to find this information.
  This change allows it to get this metadata from the node binary itself,
  and thus to adapt to future changes made to the layout of the ScopeInfo
  data structure.

  BUG=

  R=bmeurer@chromium.org

PR: #2974
PR-URL: nodejs/node#2974
Reviewed-By: Rod Vagg <r@va.gg>
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
75fe773
@joaocgreis joaocgreis pushed a commit to janeasystems/node-v0.x-archive that referenced this pull request Sep 23, 2015
@rvagg rvagg 2015-09-22, Version 4.1.1 (Stable) Release
Notable changes

* buffer: Fixed a bug introduced in v4.1.0 where allocating a new
  zero-length buffer can result in the next allocation of a TypedArray
  in JavaScript not being zero-filled. In certain circumstances this
  could result in data leakage via reuse of memory space in
  TypedArrays, breaking the normally safe assumption that TypedArrays
  should be always zero-filled. (Trevor Norris) #2931.
* http: Guard against response-splitting of HTTP trailing headers
  added via response.addTrailers() by removing new-line ([\r\n])
  characters from values. Note that standard header values are already
  stripped of new-line characters. The expected security impact is low
  because trailing headers are rarely used. (Ben Noordhuis) #2945.
* npm: Upgrade to npm 2.14.4 from 2.14.3, see release notes for full
  details (Kat Marchán) #2958
  - Upgrades graceful-fs on multiple dependencies to no longer rely on
    monkey-patching fs
  - Fix npm link for pre-release / RC builds of Node
* v8: Update post-mortem metadata to allow post-mortem debugging tools
  to find and inspect:
  - JavaScript objects that use dictionary properties
    (Julien Gilli) #2959
  - ScopeInfo and thus closures (Julien Gilli) #2974

PR-URL: nodejs/node#2995
4a6f1fe
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Sep 23, 2015
@misterdjules misterdjules deps: backport 357e6b9 from V8's upstream
Backport 357e6b99ee3927cc075dd8d27c99b89d858f9dd5 from V8's upstream.

Original commit message:

  Add ScopeInfo constants to post-mortem metadata

  mdb_v8, a post-mortem debugging tool for Node.js, allows users to
  inspect ScopeInfo structures in order to get more information about
  closures.

  Currently, it hardcodes the metadata it uses to find this information.
  This change allows it to get this metadata from the node binary itself,
  and thus to adapt to future changes made to the layout of the ScopeInfo
  data structure.

  BUG=

  R=bmeurer@chromium.org

PR: #2974
PR-URL: nodejs/node#2974
Reviewed-By: Rod Vagg <r@va.gg>
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
b93ad5a
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Sep 23, 2015
@rvagg rvagg 2015-09-22, Version 4.1.1 (Stable) Release
Notable changes

* buffer: Fixed a bug introduced in v4.1.0 where allocating a new
  zero-length buffer can result in the next allocation of a TypedArray
  in JavaScript not being zero-filled. In certain circumstances this
  could result in data leakage via reuse of memory space in
  TypedArrays, breaking the normally safe assumption that TypedArrays
  should be always zero-filled. (Trevor Norris) #2931.
* http: Guard against response-splitting of HTTP trailing headers
  added via response.addTrailers() by removing new-line ([\r\n])
  characters from values. Note that standard header values are already
  stripped of new-line characters. The expected security impact is low
  because trailing headers are rarely used. (Ben Noordhuis) #2945.
* npm: Upgrade to npm 2.14.4 from 2.14.3, see release notes for full
  details (Kat Marchán) #2958
  - Upgrades graceful-fs on multiple dependencies to no longer rely on
    monkey-patching fs
  - Fix npm link for pre-release / RC builds of Node
* v8: Update post-mortem metadata to allow post-mortem debugging tools
  to find and inspect:
  - JavaScript objects that use dictionary properties
    (Julien Gilli) #2959
  - ScopeInfo and thus closures (Julien Gilli) #2974
ab55b45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment