exitOverride doesn't override call to process.exit

[nicholasbailey]

I'm running into an issue where exitOverride doesn't work the way I would expect it to based on documentation, though the behavior may be intended.

My assumption is that exitOverride would completely override the exit behavior of the Command, and that the provided callback would be called instead of calling process.exit. Instead, the callback is called and then, if the callback does not throw process.exit is called as well.

Here's a minimal example that will demonstrate this

import {Command} from 'commander'

let happy = false
const program = new Command()
    .exitOverride((err) => {
        happy = true
    })

program.parse([], {from: 'user'})

// This line will never be hit, and the process will exit with exit code 1
expect(happy).toBe(true)

The issue seems to be this snippet

  _exit(exitCode, code, message) {
    if (this._exitCallback) {
      this._exitCallback(new CommanderError(exitCode, code, message));
      // Expecting this line is not reached.
    }
    process.exit(exitCode);
  };

This looks like it should be an if-else.

If this is expected behavior, please close this (though it might be nice to update the documentation to cover this, since it confused the heck out of me).

[shadowspawn]

This is intentional. Sorry you were confused.

You can override the exit behaviour, but Commander code still expects the normal flow of execution to stop. The default behaviour for exitOverride is a throw. To enforce breaking the normal flow of execution in case the override returns (as in your example), _exit falls through to the original exit call.

Hmm, maybe in the case the exit override returns it could throw instead. That might make it clearer that the exitOverride() had achieved something, albeit not the desired outcome in your case?

[nicholasbailey]

I'd say it's probably the same either way - the use case for not exiting was testing, and handling an exception as part of a test of an error path seems like an entirely reasonable workaround. Thanks for the clarification!

[nicholasbailey]

PS: Big fan of the library API/design overall.

[shadowspawn]

An answer was provided, and no further activity in a month. Closing this as resolved.

Feel free to open a new issue if it comes up again, with new information and renewed interest.

[shadowspawn]

This came up again in #2092 and I had a go at making this clearer in the README: #2098