-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix spawning of Node.js binaries (.cmd files) on Windows #6
Conversation
@@ -3,7 +3,16 @@ var spawn = require('child_process').spawn; | |||
function Spawn(stream, command, args, name, output) { | |||
var exited = false; | |||
var ended = false; | |||
var spawnedProcess = spawn(command, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep spawnedProcess
initialized at the top of the method here (keep undefined):
var spawnedProcess;
lib/Spawn.js
Outdated
}; | ||
} | ||
|
||
var spawnedProcess = spawn(command, args, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping with above comment remove var
:
spawnedProcess = spawn(command, args, options);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few nits before approval.
lib/Spawn.js
Outdated
if (process.platform === 'win32') { | ||
options = { | ||
// required so that .bat/.cmd files can be spawned | ||
shell: process.env.comspec || 'cmd.exe', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove trailing comma
Made requested changes. Do you want me to rebase and squash these in a single commit? |
Looks good! No need to squash. I can do that over here. For now let's merge and I'll find a way of testing before publishing a new release. Thank you for your help! |
Happy to help :) |
This PR fixes running .bat/.cmd files on Windows (and .cmd files are used to wrap Node.js binaries installed via npm and yarn) by setting
shell
options ofchild_process.spawn()
call on WIndows only.Fixes #5