Output stream

[Melissa0x1f992]

Pull Request

Add custom output streams to commands

Problem

stdout isn't available to my user. I have a custom method of displaying text output to the user. I still want to make use of the full Commander help suite. Disabling the built-in help commands and writing wrappers becomes an increasingly complicated task when you want to make use of the entire help suite.

Solution

This PR will solve that problem by removing the assumption that all output is to process.stdout. A user can instead pass a writeable stream that will replace the default stdout stream. In this way, anyone can output command results in any way that works for their use case by defining their own writeable stream.

This is a more elegant solution to the problem in #1370. Instead of using helpOption(false) and writing my own help option, I can use Commander's help option, and handle the output with my own writeable stream. This is a less invasive solution than #779, as it leaves the implementation of the writeable stream to the user.

Things to note:

• I've intentionally not updated the README or the examples/. This is a WIP PR, and I'd like to know if this idea will be accepted before documenting it further.
• This adds a dependency on stream.Writeable, but that's built into Node, so I think that's fine
• process.stdout is a stream.Writeable, but it's also a tty.WriteStream. Only the latter has the .columns field in its API. Nonetheless, I've chosen to work with stream.Writeable for a few reasons.
  First, tty.WriteStream requires a file descriptor passed to its constructor, which is too literal/constraining of an implementation for this idea; not every Commander output stream should need a file descriptor.
  Second, all of Commander's uses of .columns are already protected by a default if the value is Falsey.
  Finally, if someone wants a non-default column width in their custom stream, they can specify a .columns field themself.
• An alternative way to address the above point would be to create a type that's just a stream.Writeable + .columns. I think this alternative adds unneeded complexity. Also, under this alternative, I don't know how the default process.stdout would fit in, as it wouldn't be of the custom type, even if it was compatible. However, this alternative would have the pro of not relying on the existing .columns || 80 in the code to prevent undefined behavior.
• Error output still prints to process.stderr by way of console.error(). For my use case, I have no problem with this, but it would be straightforward to expand with an errorStream() method and field if we wanted.
• Similarly, currently Commander doesn't use console.log() for standard output. From this point forward, that would change from an implementation detail to a requirement.

ChangeLog

• allow overriding the output from default (process.stdout) to any stream.Writeable

[shadowspawn]

A couple of things are in flight that are relevant. I am interested to dig through the things you considered.

I played around with custom stream idea when working on #1296 but settled on a light weight option which is just a simple "write" routine. I left supplying a custom logger undocumented as it wasn't the focus of the work, and I didn't have use cases to check it against.

There is a big refactor fo the help in #1365. This will make it more practical to override help to change behaviour

[cravler]

For me it will be cool if we have something like this:

class Command {

  /**
   * Return output stream
   * https://nodejs.org/api/stream.html#stream_class_stream_writable
   *
   * @returns {stream.Writable}
   */

  outputStream() {
    return process.stdout;
  };

  /**
   * Return error stream
   * https://nodejs.org/api/stream.html#stream_class_stream_writable
   *
   * @returns {stream.Writable}
   */

  errorStream() {
    return process.stderr;
  };

  /**
   * @param {string} message human-readable description of the error
   * @return never
   * @api private
   */

  _error(message) {
    if (!this._exitCallback) {
      this.errorStream().write('error: ' + message + '\n');
    }
  };

}

and instead of using console.error('error: something') use this._error('something')

[shadowspawn]

Some current code patterns that complicate the output customisation are:

• help may be displayed on stdout or stderr (new in 7)
• help is displayed with process.stdout.write (and stderr), while errors are displaying using console.error
• .helpInformation() has a trailing linefeed (and displayed using write)

I was tempted to remove the trailing linefeed from .helpInformation() when it was made public so it could more conveniently be displayed with either log or write, but kept it the same at the time for backwards compatibility.

[cravler]

for example, if I want to show all errors with date prefix and in red color:

const { Writable } = require('stream');

const errStream = new Writable({
  write(chunk, encoding, callback) {
    const str = chunk.toString();
    const prefix = '[' + (new Date()).toISOString() + '] ';
    const applyStyle = str => '\x1b[31m' + str + '\x1b[0m'; // red
    process.stderr.write(applyStyle(prefix + str));
    callback();
  }
});

[Melissa0x1f992]

There is a big refactor for the help in #1365. This will make it more practical to override help to change behaviour

Looking at this refactor, I think the out and err stream customization would get used in the _getHelpContext() method. Would it be easier if this commit had branched from something closer to that code? Lesson learned that I should have developed from the develop branch 😅

I think @cravler's example shows how letting the user provide their own stream opens up a lot of customization without having to bloat Help with features. My use case involves displaying text to users completely away from a terminal environment. It'd be out of scope to have Commander directly support such diverse use cases, but just by accepting streams from the user, we get the ability to integrate Commander to a broad set of use cases.

I agree that it's better to replace console.error() with .write()ing to an err stream. That would keep the console output story consistent.

[shadowspawn]

I think using an explicit class like stream.Writeable is too strong when all that is being used from the class is a simple call to one method. Bit torn whether to allow both console.log() and process.stdout.write() styles, implicit line-feed and explicit line-feed. Over-the-top support would be for plain function or object-with-function (to still allow passing object), with either style! For example:

program.setOutput({
  log: (str) => console.log(str),
  error: (str) => console.error(str)
});

program.setOutput({
  log: console, // uses console.log
  error: console // uses console.error
});

program.setOutput({
  writeOut: (str) => process.stdout.write(str),
  writeErr: (str) => process.stderr.write(str)
});

program.setOutput({
  writeOut: process.stdout, // uses process.stdout.write
  writeErr: process.stderr // uses process.stderr.write
});

[shadowspawn]

Hmm. I wanted to try out the object-with-function approach to mirror passing a typed class/object, but at least in my attempt, I think it is less clear than a simple function and didn't add much value. Potentially works better from TypeScript than JavaScript as the duck typing can be stronger.

[cravler]

I vote for this:

const util = require('util');

program.setOutput({
  log: (...args) => process.stdout.write(util.format(...args, '\n')), //same as console.log
  error: (...args) => process.stderr.write(util.format(...args, '\n')) //same as console.error
});

[shadowspawn]

columns and tty got mentioned in first comment, and I realise there are a few issues in release/7.x. Old code is using stdout.columns even when displaying on stderr now, and should probably test isTTY too. I haven't worked out how to improve the default behaviour yet. Hopefully solution will mean this PR does not need to directly deal with it.

[shadowspawn]

I'm going to have play around with this signature. Because of the changes to help, I think people doing custom solutions affecting the help will need to supply both stdout replacement and stderr replacement. The columns property will allow number or function, so can plug in runtime calculation if needed.

outputOverride({
  log: (str) => console.log(str),
  error: (str) => console.error(str),
  columns: 120
});

(This could be used to add timestamp to error calls as in the example above, but that would affect help being displayed on stderr too. A bottleneck routine for non-help errors is probably easier for that.)

[cravler]

I'm going to have play around with this signature. Because of the changes to help, I think people doing custom solutions affecting the help will need to supply both stdout replacement and stderr replacement. The columns property will allow number or function, so can plug in runtime calculation if needed.

outputOverride({
  log: (str) => console.log(str),
  error: (str) => console.error(str),
  columns: 120
});

(This could be used to add timestamp to error calls as in the example above, but that would affect help being displayed on stderr too. A bottleneck routine for non-help errors is probably easier for that.)

For me it looks wrong to write help to stderr, help must always go to log and errors to error and if it is needed I can override log to use stderr.

[cravler]

For version output we can add:

outputOverride({
  printVersion: (version) => console.log(version)
});

[shadowspawn]

For me it looks wrong to write help to stderr, help must always go to log and errors to error and if it is needed I can override log to use stderr.

In the normal case when the user requests the help, yes it goes to stdout.

When the help is displayed in response to a usage error it is suggested to use stderr so the help does not get piped into another command or into a file as though it were valid command output. It is being displayed as part of a diagnostic message for the usage error:

• https://www.jstorimer.com/blogs/workingwithcode/7766119-when-to-use-stderr-instead-of-stdout
• https://unix.stackexchange.com/questions/8813/should-the-usage-message-go-to-stderr-or-stdout

[shadowspawn]

Naming: or maybe configureOutput to match configureHelp.

[cravler]

Sorry, maybe i explain wrongly. Maybe this will be more clear:

configureOutput({
  help: (str, error = true) => console[error ? 'error' : 'log'](str),
  error: (str) => console.error(str),
  version: (str) => console.log(str)
});

[shadowspawn]

Oh, I see what you mean now. I was thinking of log = console.log = stdout , and error = console.error = stderr. Splitting up the types of output is another way (i.e. help, error, version).

[shadowspawn]

My current experiment to allow overriding output and being careful about columns has:

    this._outputConfiguration = {
      write: (arg) => process.stdout.write(arg),
      writeError: (arg) => process.stderr.write(arg),
      // columns is used for wrapping the help
      getColumns: () => process.stdout.isTTY ? process.stdout.columns : undefined,
      getErrorColumns: () => process.stderr.isTTY ? process.stderr.columns : undefined
    };

[shadowspawn]

This PR was a good trigger for discussion and development. Closing in favour of #1387, with a key difference being weaker typing than using an explicit stream.

Thank you for your contributions.