Npm exec implementation for #3313 #4058

Closed
wants to merge 5 commits into
from

Projects

None yet

6 participants

@can3p
can3p commented Oct 30, 2013

Hi

I've extracted the exec code from the lifecycle.js and added an npm exec command to run scripts in the package context. In part it fixes #3313. In reality I couldn't eliminate lifecycle at all, because it contains some specific logic like hooks.

I know that @grncdr works on the same task, but it happened that I've made this work too and I'll be happy if it will be useful.

I'm ready to fix things if needed.

@grncdr
grncdr commented Oct 30, 2013

I'm not too attached to my implementation, and haven't had a lot of spare time to work on it. I may come back to it later, but I got distracted by my desire to have a more faithful sh interpreter. For me, that's the most important part of what I was doing, as it would allow scripts (and exec commands) to be portable to environments that don't include a posix shell (windows).

@can3p
can3p commented Oct 30, 2013

I'm not sure that I got your point.

When I was working on the implementation my intention was to implement exec command in such a way that it will run the commands in the same context as run-script did but allow me to specify the arguments for the command.

If you're talking about the same then what will be different on windows in this case?

@grncdr
grncdr commented Oct 30, 2013

It turns out I was quite ignorant of what windows cmd.exe supports. I didn't know it already supported command chaining with boolean operators (||, &&) and output redirection, if that's the case there's really less of a need for the bashful integration I was working on and I don't see any compelling reason to prefer my patch.

@grncdr
grncdr commented Nov 17, 2013

If you're talking about the same then what will be different on windows in this case?

I was mostly talking about more complicated scripting support such as command line chaining, output redirection etc. (See http://substack.net/task_automation_with_npm_run for examples of the kinds of things that I'd like "scripts" to support cross-platform). In the interest of getting those sorts of scripts to work in windows (without having to require an out-of-band dependency on bash) I integrated bashful into my npm-exec repo and was using that in npm to replace most of the lifecycle code (much like what you've done here).

However, bashful is not complete, and I was working on making it more complete when you opened this PR. Seeing that windows cmd.exe supported some useful shell operators, I started to think that maybe the bashful integration was not as useful/important after all.

But today I found out that cmd.exe doesn't support backgrounding jobs with &, and you can't have multiple commands separated with a semicolon. (E.g. try echo hello; echo hello2 in both cmd.exe and any posix shell). IMO these are common enough operations that they justify using bashful (or something like it) to implement npm exec (and run npm scripts etc) so I'm going to start back up on bashful.

@can3p
can3p commented Nov 19, 2013

@isaacs, can you define a functionality that is sufficient in this case?

@ricardobeat ricardobeat referenced this pull request Feb 10, 2014
Closed

Add command npm-exec #4554

@domenic
Member
domenic commented Feb 24, 2014

I really want this. Hopefully we can put this at the top of @isaacs's priority queue to review.

@jeffmo
jeffmo commented May 6, 2014

I also really want this. What can I do to help move it forward?

@mgol
mgol commented Jun 16, 2014

This would be a godsend to Grunt users. It would make it extremely easy to migrate from grunt task to npm run task; no more excuses to install anything globally!

@othiym23
Collaborator

/cc @bcoe There's a bunch of useful stuff that could be incorporated from this into npm-exec as well. @can3p, we sort of routed around this PR in favor of #5518, but there's a lot of good stuff in here! Thanks for the work!

@othiym23 othiym23 added the npm-exec label Jul 24, 2014
@othiym23
Collaborator

This is another patch that got overtaken by events elsewhere in the npm project. I'm sorry we let it sit for so long. A few things have happened:

  • npm@2 added the ability to pass arguments to commands defined in "scripts"
  • a few of us agreed that it would be handy for npm to include a separate command, npm-exec, that allows you to run commands in the environment provided to child processes by run-script, but without all of the command-line argument parsing that regular npm does. This is in part because decoupling that argument parsing from npm for a single command was deemed sufficieintly complicated that it made more sense to put in a separate command.
  • many tweaks have been made to npm run-script since the release of npm@2, so this has drifted further from where the code is now.

There is clearly still demand for an npm-exec, so if you wanted to take this code and use it as the basis for that and open it as a new PR, that would be awesome. 🎆 This patch, however, is dead – npm exec isn't something that makes sense right now, and the code here and the code in master have gone their separate ways. I apologize for letting this patch sit so long without feedback from the core team, and I thank you for taking the time to put together such a complete patch.

@othiym23 othiym23 closed this Feb 27, 2015
@othiym23 othiym23 removed the review label Feb 27, 2015
@ronak11 ronak11 referenced this pull request Aug 27, 2015
Closed

ENOENT errno -4058 #7393

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