-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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(core): combine serial and parallel execution + forward sigint to child process #13885
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Is this also related to (or even fixes) #11782? Maybe it needs to be expanded to run-script as well? |
@luxaritas this fixes the |
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 looks like a good change, but there are some weird formatting issues. I'll reformat it and address the minor comment I had, but this should be merged later today :)
@@ -208,6 +216,7 @@ function createProcess( | |||
const processExitListener = () => childProcess.kill(); | |||
process.on('exit', processExitListener); | |||
process.on('SIGTERM', processExitListener); | |||
process.on('SIGINT', () => childProcess.kill('SIGINT')); |
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.
Lets reuse the childProcessExitListener
here
Huh, on second glance I don't have permission to push up to the branch on your fork. Can you run |
Any chance we can get run-script addressed now as well? |
@AgentEnder oh that's weird. I thought contributors had push access to forks. Will do! |
@luxaritas I'm currently traveling, so I don't think I'll have time for going through |
@AgentEnder thanks for fixing tests and pushing this over the finish line! Let me know if there are any pending changes that you need help with. I'm now back so I can spend some time on it |
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Attempt at fixing #13766
Combined parallel and sync process spawning since parallel one was already dealing with capturing signals, so it seemed like the easiest way to fix it