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
Chore: make shelljs a devDependency instead of a dependency #8608
Conversation
@@ -53,7 +53,7 @@ function installSyncSaveDev(packages) { | |||
if (Array.isArray(packages)) { | |||
packages = packages.join(" "); | |||
} | |||
shell.exec(`npm i --save-dev ${packages}`, { stdio: "inherit" }); | |||
childProcess.execSync(`npm i --save-dev ${packages}`, { stdio: "inherit", encoding: "utf8" }); |
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.
Have you tested this on windows? Edit: I see you mentioned this the PR, sorry.
I always have trouble using childProcess.exec()
on windows with node packages or batch file.
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 tested it in Windows (cmd/ps) and Windows 10 Bash both work as expected.
It is childProccess.spawn()
that has problems with running batch files but childProcess.exe()
runs in a command prompt or shell that allows it to run normally.
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 confirmed that the execSync
works on Windows.
If my memory is correct, spawn
npm
does not works on Windows but exec
is OK.
The shelljs module is quite large, and we only use it for a few simple operations that can easily be replaced with standard library functions. This commit makes shelljs a devDevDependency instead.
b7db75d
to
9ec414f
Compare
LGTM |
Edit: We've had a few folks test in Windows already, feel free to merge. |
What is the purpose of this pull request? (put an "X" next to item)
[x] Other, please explain:
What changes did you make? (Give an overview)
The shelljs module is quite large, and we only use it for a few simple operations that can easily be replaced with standard library functions. This commit makes shelljs a devDependency instead.
Is there anything you'd like reviewers to focus on?
Our tests for
npm-util
are not very thorough -- they use method stubs and don't verify whether a command was actually executed successfully. We should probably make them sturdier in the future.Can someone that uses Windows double-check that the
npm install
command works when usingeslint --init
and selecting a popular styleguide? I've tested it on macOS, but I want to make sure it works on Windows as well, since I know there are somechild_process
quirks between platforms.