Skip to content
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

Ensure that child build processes started when running on Windows exit properly. #1022

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

pkaminski
Copy link
Contributor

@pkaminski pkaminski commented Sep 10, 2020

Changes

plugin-run-script and plugin-build script use execa.command with the shell: true option set to execute the user's command. On Windows, this causes the child process to remain running after Snowpack exits: sindresorhus/execa#433. The fix is to override the default windowsHide option -- as Snowpack always runs from a console we don't actually need execa to hide any extra console windows for us.

Testing

Tested on my project's build with a few plugin-run-script instances by keeping an eye on the process list before/after running Snowpack. I don't immediately see how to automate testing the issue...


This change is Reviewable

…t properly.

plugin-run-script and plugin-build script use execa.command with the shell: true
option set to execute the user's command.  On Windows, this causes the child process
to remain running after Snowpack exits: sindresorhus/execa#433.
The fix is to override the default windowsHide option -- as Snowpack always runs from
a console we don't actually need execa to hide any extra console windows for us.
@pkaminski pkaminski requested a review from a team as a code owner September 10, 2020 01:46
@vercel vercel bot temporarily deployed to Preview September 10, 2020 01:46 Inactive
@vercel
Copy link

vercel bot commented Sep 10, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/hyx19jjxl
✅ Preview: Canceled

@FredKSchott
Copy link
Owner

+1 as long as you can confirm that this doesn't make the general user experience any worse. You're saying that windowHide doesn't make a difference here since you run snowpack as a CLI anyway? That doesn't change if you run Snowpack in some integrated terminal, like a terminal built-into your code editor?

@stramel would love your help testing this manually on windows if you can!

@pkaminski
Copy link
Contributor Author

FWIW, I tried running Snowpack (with the patch) from VSCode's built-in terminal and everything worked fine: no anomalous windows popped open, and all processes exited correctly.

Even if there is some corner case where it makes the UX worse, though, I think the alternative of leaving a zombie Node process around for each run or build script configured every time you run Snowpack is worse. I had 30+ of these zombies by the time I coincidentally happened to check my process list... The only other option I can think of is to find some better fix than the execa folks could, but that is definitely beyond my skills.

@FredKSchott FredKSchott merged commit 1667143 into FredKSchott:master Sep 11, 2020
@FredKSchott
Copy link
Owner

LGTM! Thanks for investigating this fix 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants