Best practice for error output?

[elliot-nelson]

I find myself repeating a pattern with most of my CLI programs, curious if there is an alternate take.

Usually there are 3 classes of CLI errors:

1. Argument/command line validation error - in this case, I probably want to detect it early, in check(), and generate usage details + error via cliMessage.

2. Expected error cases - in this case, I don't want usage help, but would like to print an error message with no stack trace.

3. Unexpected error cases - an exception (from my code or a library) bubbled up, in this case I want a full stack trace for debugging.

Here's a complete example:

class SimpleError extends Error { }

sywac
    .command({ /* details */ })
    .command({ /* details */ })
    .parse().then(result => {
        if (result.errors && results.errors[0] instanceof SimpleError) {
            console.error(chalk.red(result.errors[0].message));
        } else if (result.errors && result.errors[0]) {
            console.error(chalk.red(result.errors[0].stack));
        } else if (result.output) {
            console[result.code === 0 ? 'log' : 'error'](result.output);
        }
    })

(The parseAndExit nicely handles the last part for you, but you can't insert your custom error class handling above if you use that.)

I'm curious if this tracks for you, creating a token "error" class in every CLI program so you can distinguish between expected and unexpected errors. I've created this token error class in so many different utilities and libraries that I sometimes wish the concept of a UserError - a thrown Error containing a message intended for the user - existed in core nodejs, and then sywac could handle it for me by printing the .message if it's a UserError or the .stack if it's not.

[nexdrew]

I think your 3 error scenarios are valid and well summarized.

Sywac attempts to handle number 1 and number 3, but it doesn't really handle number 2. If your program code throws an error for number 2, then sywac handles it as number 3. If your program code uses context.cliMessage() for number 2, then sywac handles it as number 1.

In my CLI programs, I guess I try to avoid number 2, but when I've needed it, I'm typically content to treat it as number 1 or number 3. There have been times when I've let the command handler do its own error logging and program short-circuiting, but that's probably not the best way to do it, especially if (a) the program needs to do some resource cleanup after a command was attempted or (b) the program is a server-side chatbot.

We could include a special Error type as part of sywac that a CLI author could use/throw for the expected error scenario of number 2, so that the framework could handle it without help text or a stack trace, as you describe. What do you think of that idea?

[nexdrew]

I should note, however, that you can use context.unexpectedError('Some message') to achieve the desired effect of number 2 (without the need for a special Error class).

[nexdrew]

It's also possible to just throw 'Some message', which also achieves the effect of number 2, but I don't recommend doing this.

[elliot-nelson]

I should note, however, that you can use context.unexpectedError('Some message') to achieve the desired effect of number 2 (without the need for a special Error class).

Ah, I just looked into that. It looks like that would be useful in simple inline commands (where you have access to sywac's context).

[elliot-nelson]

We could include a special Error type as part of sywac that a CLI author could use/throw for the expected error scenario of number 2, so that the framework could handle it without help text or a stack trace, as you describe. What do you think of that idea?

This is actually what I'm angling for although full disclosure, I'm not sure if it's even a good idea.

Typically, I'm working in a CLI app with several commands that all call various sub modules and each of these sub modules could have pretty deeply nested errors that are still "user-fixable" where a stack trace is not desirable. I guess an example is something like:

const HelpError = require('../errors').HelpError;
// in a module 3 levels divorced from the CLI
throw new UsageError([
    'It looks like your git config is not set properly.  Run the following command and try again:'
    '',
    '    git config --global acme.user.apitoken <your api token>',
    ''
].join('\n'));

In this case it's "worth it" to create an Error class used by several modules that denotes "don't show a stack with this one".

If sywac included such an Error, that would be nice but now a lot of "pure" (non-CLI) modules, that have a specific purpose, need to include a command-line module:

const StacklessError = require('sywac').StacklessError;

(I realize me calling my module "pure" is kind of laughable considering the clearly CLI-formatted advice it is throwing. I guess to be truly "pure", my modules should be throwing a GitConfigNotCompleteError, and I should be catching that at the very bottom of my sywac CLI program and choosing not to print a stack then.)

Other ideas I've thrown around are goofy string-parsing things, like prefixing all of my "helpful text" errors with a string, and using that as a marker. E.g.:

throw new Error(':: Aborted, try again.');

[elliot-nelson]

I think what I'd want most from sywac is not to dictate how I tell the difference between errors in Camp 2 and Camp 3, but just to let me do it without resorting to parse().

In other words, a tidbit like this:

sywac
    .command({...})
    .errorFormatting(err => err.message.startsWith('::') ? err.message : err.stack)
    .parseAndExit()

Now I can be as clever or as stupid in my own CLI program as I want, but sywac still lets me print errors in all 3 categories using parseAndExit. (The real "errorFormatting" handler could be a style() option perhaps, or maybe a property of displayOptions()?)

EDIT: Actually I'm not 100% sure that I can distinguish between Error 1 and Error 3 if I'm specifying showHelpByDefault(), I'd have to play around with it I suppose. For this to work I'd want to always show help if I intentionally add a cliMessage in check(), but never if I throw.

[nexdrew]

Yeah, sounds like it would be best to allow the CLI author to tell sywac one (or more) of the following:

1. This is the error type(s) I'm going to throw for which you should not print the stack trace

   Perhaps something like .registerExpectedError(CustomError) or .registerExpectedError([CustomXError, CustomYError])...

2. This is how I want to handle errors when they are caught

   Perhaps something like .handleError(someFunction), not sure exactly what the contract for this handler should be...

Or, as you suggested, this is the "style" I want you to use when formatting a caught error.

I'd be willing to support any or all of these ideas.

[elliot-nelson]

I'll noodle on it too!

The fly in the ointment is the non-command API which would usually be parseAndExit chained into a then block with your main function. In this case, Camp 2+3 errors are out of sywac's hands (it will only handle Camp 1).

I think in my ideal world, not only would we add some form of synchronous configuration you mention above, but also an asynchronous re-entry point for handling errors:

sywac
  .string(...)
  .boolean(...)
  .parseAndExit()
  .then(argv => {
    // my simple program
  })
  .catch(sywac.handleErrors)

This way even this type of program could automatically have sywac use your style's messages style for the error, for example.

[elliot-nelson]

OK, I've been thinking about this, and I have an idea - let me know what you think.

One of the reasons I became really attracted to sywac in the first place is that when using commands, there is a great symmetry for unit testing. You can define an entry point like this:

// src/cli.js
module.exports = require('sywac')
    .command(blah)
    .command(blah)
    .strict()
    .showHelpByDefault();

Now that you've defined your entire program, you can write your binary:

// bin/myprogram
#!/usr/bin/env node
require('../src/cli').parseAndExit();

And your unit test:

// spec/cli-spec.js
it('should work', function () {
    const result = cli.parse('some arguments');
    // various expectations on the .code & .output of your program
});

This beautiful symmetry becomes broken, unfortunately, if you are NOT using commands. Your code doesn't get hit until after the parse/parseAndExit call is complete, which means you cannot structure your program exactly this way - you need an additional wrapper.

My proposal is to make the non-command version more like the command version by allowing (probably optionally?) the user to "define" their program as a root-level run command.

/* Instead of this */
require('sywac')
    .string('foo')
    .parseAndExit()
    .then(argv => /* my program */);

/* Let's allow this */
require('sywac')
    .string('foo')
    .run(argv => /* my program */)
    .parseAndExit();

This allows you to retain that "either execute, or test" symmetry that I love whether you are using commands or not, AND, it let's a new error formatter (which I'm tentatively thinking will end up being styleOptions.formatError) behave the same way in both cases, since it is book-ending both the same way.

What do you think?

[nexdrew]

For most of my non-command CLIs, I try to organize my package into two main parts - one is a library module intended to be used via require(), where most of the program logic goes, and the other part is a CLI wrapper around the library module that makes it convenient to use on the command line. The CLI wrapper is typically only responsible for translating input (i.e. converting argv to function arguments/options), executing the library (i.e. calling a function), and then translating the result to output (i.e. printing something to stdout or stderr).

In that type of structure, I typically write tests to cover the primary use-cases by executing the CLI in a separate process (using execa or just straight up child_process) and making assertions on the exit code and stdout/stderr output. This generally covers testing both parts of my package. Sometimes, however (typically for code coverage purposes), I'll also write tests that just use the library module in specific ways.

When comparing this approach to what you describe, the crux seems to be the necessity to execute the actual bin/executable in a separate process for testing purposes (which, quite honestly, is sometimes a pain in the butt).

I agree that it would be nice to just require() my CLI module and call .parse('some arguments') and have it run all the logic that would typically come after the call to .parseAndExit() - and so, for this reason, I think I'm onboard with your idea - but it's still not exactly the same as running the bin/executable because (1) you won't be able to assert that the output went to the correct IO stream (unless we change sywac's result to somehow make this distinction) and (2) you aren't actually testing/covering the code in the bin/executable module (the one that calls .parseAndExit()).

I've always viewed testing the actual bin/executable module as a necessary evil, and I'd love to come up with a standard way to make this super easy (though I don't know of a way to do this without writing a separate opinionated package), but I think both of these different approaches (the one you describe and the one I describe) are valid and don't need to be mutually exclusive when it comes to being supported by sywac.

So, long story short, I'd like to explore your idea of adding something like .run() to sywac's API and see how it plays out.

Thanks for the great feedback! I think you've got some really good ideas.

[elliot-nelson]

Thanks much!

Re: testing the bin file: if I were to write a "Testing Guide" for sywac.io someday, what I'd want to recommend to people is kind of a hybrid:

1. At the outermost layer, create the smallest possible executable you can (the one with the actual hashbang in it). With sywac you can have this be 1 line long and you may never need to touch it again. You can test it by writing a single integration test in spec somewhere (i.e., you can exec that file just once, perhaps with -h, and confirm it returns exit code 0 and some help output).

2. That frees you up to write nice, in-process unit tests for src/cli.js using .parse(). I think the fact this is so easy and nice-looking is a big bonus because it discourages too much decoupling. Being able to easily say that .parse('--devices a,b,c') calls myDatabase.update({ devices: ['a', 'b', 'c'] }) is great, because you don't get caught unawares by bad assumptions. *

3. Then for larger programs, a step in from your src/cli.js or your command submodules, of course you'll write non-CLI-specific unit tests for modules that handle stuff like calling out to APIs, reading/writing files, doing whatever this CLI does. (This is optional for programs simple enough to do all of their logic write in the run handlers of their commands, imo.)

* In particular, the "bad assumptions" is code like if (argv.devices). If you don't know that sywac sets a default of [] for argv.devices if it is of type array, and you directly unit test only your command's run handler, then your unit test doesn't test the actual behavior of the program. (I know I'm preaching to the choir because right now you prefer actually executing the thing, but this is stuff I'd want to put in an opinionated "How to test CLIs, the right way" section of sywac.io, if it ever came to exist.)

[nexdrew]

Yeah, that makes sense. Using in-process tests gives you a higher degree of flexibility and control over the testing environment (e.g. injection), so I would prefer to use that (instead of executing a CLI via child process) whenever possible.

[elliot-nelson]

An interesting aside I discovered today: you actually could get exactly what I want, with no changes to sywac:

class StacklessError extends Error {
  constructor(message) {
    super(message)
    this.name = ''
    this.stack = ''
  }
}

Because of how Error's toString() works, setting name and stack to empty string will result in the expression String((err && err.stack) || err) returning the original message, which means you can throw anywhere:

throw new StacklessError('Hello user, please try again later.');

// => Hello user, please try again later.

Suggesting that the user create their own copy of this "StacklessError" and throw it, if they'd like sywac to print a stackless error on their behalf, is another possible strategy.

[nexdrew]

Concerning your last comment about defining a StacklessError:

That's an interesting approach, but I wouldn't recommend that developers override basic Error semantics (e.g. throwing away the stack and name) just to compensate for an implementation in sywac that could/should be improved.