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.

Sign up for GitHub

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

Jump to bottom

[Fixes #388] ensure broccoli-cli exits with the appropriate status co… #456

Closed
wants to merge 1 commit into from
Closed

[Fixes #388] ensure broccoli-cli exits with the appropriate status co… #456

wants to merge 1 commit into from

Conversation

stefanpenner
Copy link
Contributor

…de on build failure

stefanpenner
stefanpenner commented Feb 29, 2020
View reviewed changes
lib/cli.ts
@@ -173,6 +176,9 @@ export = function broccoliCLI(args: string[], ui = new UI()) {
watcher.on('buildFailure', (err: any) => {
ui.writeLine('build failure', 'ERROR');
ui.writeError(err);
if (!options.watch) {
process.exitCode = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: test this scenario

@stefanpenner stefanpenner mentioned this pull request Feb 29, 2020
Closed
@peabnuts123
Copy link

peabnuts123 commented Mar 2, 2020 •
edited
Loading

Interesting. How does calling process.exit(1) mean the process exits with the wrong code? I frequently do this in my own code. What's the benefit of setting process.exitCode first?

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Mar 2, 2020 •
edited
Loading

It doesn’t, process.exitCode just a more cooperative (IMHO better) way to exit with the status code of choice. Rather than explicitly ending the process, this approach allows the process to naturally shut down, ultimately exiting with w/e the final state of process.exitCode is.

This PR is fixing a bug, specifically the bug fix here was adding a missing exitCode assignment (see above inline comment)

@peabnuts123
Copy link

Oh, I see! I had missed that there was an new exit scenario in there.

@stefanpenner stefanpenner mentioned this pull request Jul 29, 2020
Closed
@ryuran
Copy link

ryuran commented Dec 4, 2020

Something new for this PR @stefanpenner @rwjblue ?

@chiragpat chiragpat mentioned this pull request Feb 11, 2021
Merged
@stefanpenner
Copy link
Contributor Author

released as v3.5.1 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stefanpenner @peabnuts123 @ryuran @rwjblue