-
-
Notifications
You must be signed in to change notification settings - Fork 105
[Windows] Always spawn into new shell on windows + Run npm via node executable #69
Conversation
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.
The shell:
patch is 💯. I think we need to figure out something better for the other part?
child.js
Outdated
@@ -8,7 +8,7 @@ function runCommand (command, opts) { | |||
const cmd = opts.call || command || opts.command | |||
const copts = (opts.call ? [] : opts.cmdOpts) || [] | |||
return spawn(cmd, copts, { | |||
shell: opts.shell || !!opts.call, | |||
shell: opts.shell || !!opts.call || process.platform === 'win32', |
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.
ahahah Yes
index.js
Outdated
return which(opts.npm).then(npmPath => { | ||
const args = ['config', 'get', 'cache', '--parseable'] | ||
return which('node').then(nodePath => { | ||
const args = [opts.npm, 'config', 'get', 'cache', '--parseable'] |
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.
Does this actually still work if someone does --npm=npm
? 🤔 That's why this code was doing this. The opts.npm
option can be a path to the npm script itself or to the name of the binary in $PATH
.
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.
Oh, makes sense. What about we run it through this way only for windows and only if opts.npm
is absolute and/or ends with .js
?
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.
I'm fine trying this out. I think that would probably cover most cases? 🤔
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.
What about which(opts.npm).catch(() => args.unshift(opts.npm) && which('node')).then(path => child.exec(path, 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.
This would leave the current implementation untouched and just add a fallback for windows and bundled npm (and .js files in --npm switch).
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.
I think you can skip which('node')
altogether and just use process.argv[0]
, which will always point to a node executable? This otherwise sounds 👍
Thanks a bunch for submitting this!! You've been so helpful with Windows stuff and it saves me a ton of time 😻 |
@katemihalikova LGTM once tests on CI are passing~ |
I've added the fallback also to |
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.
This is some seriously stellar work and I can't tell you how much I appreciate you taking the time to sleuth through all this. I really wish I were doing a better job of treating windows well, and I'm glad it's getting the attention it needs now <3
@zkat @katemihalikova I've tested the fix on my Windows 10 machine. Unfortunately this fix is not a complete one as this only solves the issue when it's being used on a Windows' CMD. While most of the developers who code on Windows machines use some other consoles like Git Bash, ConEmu, babun, etc, they still suffer from the issue. The only version of |
@motss Thanks for the info, we know that already. This is going to be addressed in the near future. Most alternative shells used on Windows work in a way somewhere between unix-like and cmd.exe so it's difficult to sort it out sometimes.. |
Fixes for windows-specific issues #60, #58, and #62