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

Make calls to process.exit() overridable #575

Closed
bolinfest opened this issue Sep 21, 2016 · 12 comments
Closed

Make calls to process.exit() overridable #575

bolinfest opened this issue Sep 21, 2016 · 12 comments
Milestone

Comments

@bolinfest
Copy link

In general, library code should never call process.exit(). Instead, clients of the library should decide how they want to handle errors.

In particular, tying process.exit(0) to --help seems particularly bad. For a CLI parser, this may be a reasonable default, but this behavior should at least be overridable.

I am trying to build a daemon running on a UNIX domain socket that takes requests and uses commander to parse arguments just as it would if it were running in non-daemon mode. Given the way commander works today, passing --help has the side-effect of tearing down my daemon.

I have similar issues where commander makes calls to process.stdout.write() that are writing to the "wrong" stdout, from my perspective. I think the best way to secure things as a library is to remove all direct references to process from index.js and make all of its behavior injectable. It can still default to what it does today, but it should be overridable.

@bolinfest
Copy link
Author

The categorical call to outputHelpIfNecessary() (which calls process.exit(0)) from Command.prototype.parseArgs and Command.prototype.action is particularly hard to override from the outside.

@cdaringe
Copy link

cdaringe commented Oct 12, 2016

  • i agree. i feel that the issue name here could be changed to something more akin to "don't exit on .help() call"
  • further, i visited the issues section here today specifically because i run help() when something wrong has been entered. hence, i have strong feelings that the exit code should be non-zero, wherein now it is 0.

of course this is opinion. @tj, are you opposed to de-opinionification of .help()s behavior?

@cdaringe
Copy link

here's a ref to the offending code

@dovidweisz
Copy link

Doesn't the outputHelp() method cover these situations?

@ghost
Copy link

ghost commented May 10, 2018

Even though this issue is old I stumbled upon a similar issue where I am having some trouble. I am using inquirer and inquirer-command-prompt to create an interactive CLI application and wanted to use commander to parse options by continuously calling parse(userInput) in my CLI prompt.

Unfortunately though, when I run parse for a command that is missing a required argument the application writes error: missing required argument 'cmd' to the console and then invokes process.exit which exits my own application!

@tj I'd like to suggest a change wherein all calls to process.exit are replaced by an overridable function that can be freely configured. In my case I would replace it with an empty function in order to display status messages but prevent exiting.

@jacksdrobinson
Copy link

jacksdrobinson commented Aug 31, 2018

Just bumping this because I'm of the same opinion. I develop a constantly-running server app, and I can run commands on it by socketing in to an IPC handle and then parsing commands using commander. I would be happier if commander didn't call process.exit, tearing down the app.

I second @bolinfest and @NearAutomata in saying that the exit behaviour could be injected; or perhaps that parse() could return an err property on the result and the client can handle the case where result.err !== null.

@ebartz
Copy link

ebartz commented Dec 11, 2018

Did someone find any way to override code or at least ignore the exit without changing the code of commander?

@Tanja-4732
Copy link

Tanja-4732 commented Mar 19, 2019

I agree with this issue.

If you want to have a live-chat bot (e.g. for Slack or Discord) one doesn't need the bot to "clean exit".

It really came as a surprise to me, that this library shuts down your app like that.
It took me quite a while (of debugging), but after figuring out, that even a try/catch block wouldn't fix this, I discovered that there are several ... no, not throw Error()s in there ... but that it contains process.exit() numerous times!
No other library would just shut down an app! Why does this one?

Please fix this unacceptable behaviour, as it breaks many use cases.
A library should always be unassuming.

I've addressed this in further detail here: #934

@tj Please consider making the exit calls optional.

@shadowspawn
Copy link
Collaborator

I have started work on an exit override: #945 (comment)

@shadowspawn
Copy link
Collaborator

There are improvements on the develop branch for the next release (#1040).

@shadowspawn
Copy link
Collaborator

v4.0.0-0 prerelease published: #1067

@shadowspawn
Copy link
Collaborator

v4.0.0 has been released.

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

Successfully merging a pull request may close this issue.

7 participants