Add child_process.spawnShell
storzinc opened this issue · 19 comments
In the child_process module currently there is two flavors of exec function: exec and execFile, which is good, but for spawn, there is only one version, would've been great to have something like child_process.spawnShell. Currently I use this code to emulate it:
child_process.spawn('cmd', ['/s', '/c', command], {windowsVerbatimArguments:true})but it is platform specific and requires the use of undocumented flag windowsVerbatimArguments
If I understood the question correctly, what he wants a way to spawn a shell without running any commands.
I can't see the a big use case for this since you anyway need to run different commands in windows and unix. What I'm saying is that you probably need the if which this will remove from your code anyway.
The only benefit would be that you don't need to use the undocumented windowsVerbatimArguments flag. Maybe a better solution is to document that?
Actually I want to execute a shell command, but with the ability for spawned shell to inherit stdout from the current process. It is possible with spawn but not possible with exec.
Documenting windowsVerbatimArguments will also solve this issue for me, but I still think it will be better not to add platform specific flags if possible, and separate function will be better.
It's a big hole in the child_process API. You can't spawn node scripts, like npm, on windows. Ouch.
@storzinc I suggest you write an npm module that implements spawn as you'd like it to be, so it's API can get banged on and confirmed robust. I'd use it, and be happy to collaborate a bit on it, but I don't have time right now to write it.
At that point, with a solid API and implementation, I'd +1 an attempt to get it into node. As you say, we have exec-with-shell and exec-direct, why not a spawn-with-shell, as well as spawn-direct.
spawn('foo | bar > baz', { shell: true }) maybe?
I'd be OK with the above. I'd sure like to deprecate exec/execFile so that it followed the same pattern.
@piscisaureus do you think its worth PRing directly to core, or out of core first as an npm module?
I'd sure like to deprecate exec/execFile so that it followed the same pattern.
But exec(File) buffers stdout. You think that's not useful?
I'd sure like to deprecate exec/execFile so that it followed the same pattern.
But exec(File) buffers stdout. You think that's not useful?
That is useful, so please don't deprecate it.
I assumed he meant using the {shell: true} pattern instead of having two separate methods.
Indeed. With @piscisaureus 's suggestion, which I like, we would have:
- spawn: defaults to
shell: false - exec: defaults to
shell: true - execFile: identical to exec, but defaults to
shell: false
I'd prefer an API where spawn and exec both default to shell: false, but I doubt that would be accepted, too incompatible. So, the fact that exec and spawn have opposite defaults for the shell option will plague us forever.
I have made my npm module as suggested:
https://www.npmjs.com/package/child_process2
It is just repackaged child_process.js from iojs sources with added support for the shell option
Since this is now available in npm, I will close this. Thank you!
@brendanashworth I was actually hoping that it will be integrated in iojs... As for my npm module - it is relying upon iojs internals (it is just modified child_process.js from iojs itself), and as such can not be used for anything serious.
@storzinc I'll reopen - we usually have a policy of not adding to core what we have in userland, but if you'd like it to be in core that can be discussed. Would you be open to us exposing the functionality you need while keeping it as an npm module?
@storzinc your child_process2 package doesn't have any metadata to point to its github repo, so its not very useful, you can't see the source on npmjs.org.
/to @orangemocha, one of the things we were just talking about. I labelled it Windows, because it is particularly useful for windows, though spawning with a shell can be done on Unixen as well.
@sam-github you can get tarball with sources directly from npmjs.org:
http://registry.npmjs.org/child_process2/-/child_process2-2.0.1.tgz
it is based on the io.js-1.2.0 sources, but I can update it to the current version if this will help.
@storzinc, I know how to download npm tarballs. That is a difficult way to browse source, particular source that you wrote to accompany this issue. Its your call, but I suggest you make your source easier to view if you hope people will view it.
This issue kind of died. Are we going to work to port @storzinc's changes into core? Otherwise, I think this can be closed.
Depends: is it a gaping hole in the child_process API? Yes. But does that mean the issue needs to stay open without someone actively working on trying to get that hole fixed? I'm not sure, is that useful? The issue could be closed, still be in the record, and the conversation can continue.