fix(cli-hooks): stop app process if the start hook exits#2474
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2474 +/- ##
=======================================
Coverage 93.09% 93.10%
=======================================
Files 40 40
Lines 11239 11250 +11
Branches 713 713
=======================================
+ Hits 10463 10474 +11
Misses 764 764
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
WilliamBergamin
left a comment
There was a problem hiding this comment.
Thanks for putting this together 💯 the changes look good 🚀
I am wondering if instead of running the app in a separate process it would make sent to directly execute what ever is in the 'fullPath' 🤔 this way we would not need to pipe to stdout and handle process endings, let me know what you think?
|
@WilliamBergamin Super interesting callout - thanks for thinking of this and the kind review 🙏 ✨ I agree using a single process has fewer chance for these edges, but might lean toward keeping this implementation with future hopes of redirecting logs from |
🦋 Changeset detectedLatest commit: bfa23c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary
This PR stops the app process if the
starthook process exits. Fix for slackapi/slack-cli#128!Reviewers
The following commands can show existing processes remain without these changes:
List the processes spawned from the CLI command:
$ ps -e ... 89120 ttys038 0:00.29 npm exec slack-cli-start 89129 ttys038 0:00.05 node /private/tmp/asdf/node_modules/.bin/slack-cli-start 89137 ttys038 0:00.26 node /private/tmp/asdf/app.jsNotice that stopping the
starthook doesn't stop the app process:$ kill 89129 $ ps -e ... 89137 ttys038 0:00.31 node /private/tmp/asdf/app.jsNow, build and install the changes of this branch:
List the spawned processes again:
$ ps -e ... 89539 ttys038 0:01.93 slack run 89561 ttys038 0:00.31 npm exec slack-cli-start 89572 ttys038 0:00.05 node /private/tmp/asdf/node_modules/.bin/slack-cli-start 89580 ttys038 0:00.28 node /private/tmp/asdf/app.jsThen stop the hook process again:
$ kill 89572 $ ps -e ...All of the processes above should be cleaned up now!
Requirements