feat: reworking yargs API to make it easier to run in headless environments, e.g., Slack

[bcoe]

after discussion today, @nexdrew and I settled on what I think is a pretty clever extension to the parse() function, as a simple API extension that supports running yargs in headless mode (yargs with no terminal output).

---

A callback fn can optionally be provided as the second argument to .parse().
If a callback is given, it will be invoked with three arguments:

1. err: populated if any validation errors raised while parsing.
2. argv: the parsed argv object.
3. output: any text that would have been output to the terminal, had a
   callback not been provided.

// providing the `fn` argument to `parse()` runs yargs in headless mode, this
// makes it easy to use yargs in contexts other than the CLI, e.g., writing
// a chat-bot.
yargs()
  .command('lunch-train <restaurant> <time>', function () {}, function (argv) {
    api.scheduleLunch(argv.restaurant, moment(argv.time))
  })
  .parse(bot.userText, function (err, argv, output) {
    if (output) bot.respond(output)
  })

---

Long story short, parse() now accepts an optional fn, which if provided will:

1. prevent text from being output to the terminal.
2. prevent commands from being invoked if an error is thrown, or text has been output to the terminal.
3. get invoked with err, argv, output (allowing output to be routed somewhere elsewhere, .e.g., a chat-room).

reviewers: @nexdrew, @maxrimue, @lrlna

[nexdrew]

Perhaps we should add require('es6-promise').polyfill() to this file (test/yargs.js) as well.

[nexdrew]

If result instanceof Error, maybe we should pass it as the 2nd argument to usage.fail() here.

[nexdrew]

If validation fails, the following will execute (when they wouldn't before) because this is asynchronous:

1. a command handler
2. any top-level logic that runs after parsing

[nexdrew]

This is called before validation of positional args for a command, so the current exiting check isn't quite sufficient (since, if positional arg validation fails, the custom exit handler would be given a code of 0 instead of 1).

[nexdrew]

I think an API that's more "transactional", i.e. exchange one input for one output, would be more ideal than registering a custom logger and exit handler. My idea is that the input is a string or array representing a message or args to parse/execute, and the output is an object containing the following (somewhat similar to the way the checkOutput() test helper function works):

1. an overall success/fail indicator, like an integer exit code
2. an Error, if one was thrown (could possibly be used instead of an exit code)
3. a parsed argv object
4. buffered text (that would typically be output to the console/terminal) as a string or array of strings

@bcoe What do you think? How would you like to proceed?

[maxrimue]

This is so awesome! 😄

[lrlna]

@bcoe this is so ace 💯 think of all the things people can build!!

[nexdrew]

I'm mainly requesting changes for enhanced docs, the other nits are not really necessary.

Otherwise, I think it'd be cool to register a parse callback only once, as part of chained config, but it's not a huge deal since you need a new yargs instance every time anyway.

[nexdrew]

For consistency with console.log and console.error, I think we need to include line feeds too, something like:

if (output.length) output += '\n'
output += args.join(' ')

Same for _logger.error. Otherwise the spacing in output is not exactly what would have been output to the console. What do you think?

[bcoe]

Great point 👍 adding.

[nexdrew]

Since this is a private variable that is always a boolean, we could get away with just return hasOutput here.

[nexdrew]

We could possibly replace silent checks with self._hasParseCallback().

[nexdrew]

I think we need to really emphasize the use of require('yargs/yargs') and constructing/configuring a new yargs() instance for every call to parse(msg, cb). It wasn't clear to me that this was necessary, but testing discovered that it was (due to the interesting way config is reset() for commands).

[ceejbot]

Interesting-- this will mean that bot authors will need to make a little factory to make a new yargs object & configure it every time they want to parse some input. This adds a little boilerplate but the docs are clear that you need it, so no problem.

[bcoe]

@nexdrew @ceejbot okay; I took a bit more time, and without a huge amount of refactoring managed to get things working such that you can call parse() multiple times, without creating a new yargs instance each run:

const parser = require('./')
  .command('a <cat>', 'a command', function () {}, function (argv) {
    console.log('a>', argv)
  })
  .command('b', 'b command', function (yargs) {
    yargs.demand('banana')
  }, function (argv) {
    console.log('b>', argv)
  })
  .command('c', 'c command', function (yargs) {
    yargs.command('d <coolio>')
  }, function (argv) {
    console.log('b>', argv)
  })
  .help()

parser.parse('c d this-worked!', function (err, argv) {
  if (err) console.log(err)
  else console.log(argv)
})

parser.parse('a --help', function (err, argv, output) {
  if (err) console.log(err)
  else console.log(output)
})

parser.parse('c help', function (err, argv, output) {
  if (err) console.log(err)
  console.log(output)
})

I'd still like to eventually refactor some of the state logic; perhaps pulling this into a separate module called context.js, and try to ensure that all other components are stateless. I feel this can be future work however, and this current iteration is the API we all dream of.

[scriptdaemon]

@bcoe "[...] this current iteration is the API we all dream of." Almost. Would it be possible to implement a way to pass in contextual variables that get passed to the command's builder / handler? For the bot I'm implementing, the commands need to know who issued the command (for commands that respond directly to the user) and the chat in which the command was issued (for actually responding). Perhaps there's already a way (at work and don't have time to do anything with the bot atm), so I'm interested to see what ideas you have to solve this problem.

Example:

Current typical command implementation:

// Node packaged modules
import chance from 'chance';

function dice(bot) {
  const self = {};

  const steamFriends = bot.client.handlers.get('steamFriends');

  self.name = 'dice';

  self.create = (userId, chatId) => {
    return {
      'command'     : 'dice [roll]',
      'description' : 'Roll dice.',

      builder(yargs) {
        // Some commands have optional 'user' arguments that default to the user who issued the command
        yargs.option('roll', { 'default': '1e6', 'type': 'string' });
      },

      handler(argv) {
        const message = chance.rpg(argv.roll).join(' ');
        steamFriends.sendChatMessage(chatId, message);
      }
    };
  };

  return self;
}

export default dice;

Current chat message event listener implementation:

// Node packaged modules
import yargs from 'yargs/yargs';

// Local modules
import cmd from '../commands/cmd';

function runCommand(bot) {
  const self = {};

  const steamFriends = bot.client.handlers.get('steamFriends');

  function onChatMessage(message, userId, chatId) {
    yargs()
      .command(cmd.create(userId, chatId)) // Create root command, which creates all subcommands e.g. `dice`
      .parse(message);
  }

  self.name = 'run-command';

  self.create = () => {
    steamFriends.addListener('chatMessage', onChatMessage);
  };

  self.destroy = () => {
    steamFriends.removeListener('chatMessage', onChatMessage);
  };

  return self;
}

export default runCommand;

[bcoe]

@scriptdaemon I love this idea, and think it would make things a lot more powerful ... I think I have an idea as to how to add this functionality without too major of an API change. What do you think of this:

yargs
  .command('foo')
  .parse('foo --bar', {user: 'bcoe}, function (err, argv, output) {

  })

where {user: "bcoe"} would be treated like passing in a configuration file.

[scriptdaemon]

That's perfect. And if it was optional (allowing yargs.parse('command', function () { ... });) then it shouldn't require an API change at all.

[nexdrew]

I think this looks great, nice work!

For future enhancements, I agree it would be better to refactor state management (of chained configuration and context) into its own submodule, to hopefully clean up all the individual pieces of state that are modified in different places right now.

I also think it would be nice to register a single parse() callback once, something like yargs.onParse(fn), which would affect all subsequent calls to yargs.parse(msg), since apps (like chatbots) will probably use the same callback logic for every message.

Otherwise, I think this PR certainly makes yargs viable for "single config and multiple parses" functionality desirable in non-CLI use-cases. (Many apologies for being so picky about this and for dragging this out for much longer than you wanted!) Great job!

[bcoe]

@scriptdaemon I implemented your idea, allowing a context configuration object to be passed to parse(), let me know what you think. I'll be working on landing this shortly.

[scriptdaemon]

@bcoe Looks great. Since it works basically the same as .config(), that would imply it should act as an object of defaults as well if I am correct (i.e. I should be able to use the same name as an optional positional parameter, and if a user supplies a value for that parameter when issuing a command, overwrite the context value with the supplied value, yes?) I like that implementation, because that takes care of needing the context for both the builder and the handler for each command.

[bcoe]

@scriptdaemon I hadn't thought of this side-effect, but yes that totally works:

const yargs = require('./')
const parser = yargs
  .command('lunch-train [restaurant]', 'start lunch train', function () {}, function (argv) {
    console.log(argv.restaurant, argv.time)
    // api.scheduleLunch(argv.restaurant, moment(argv.time))
  })
  .help()

parser.parse('lunch-train', {restaurant: "Rudy's"}, function (err, argv, output) {
  if (output) bot.respond(output)
})```

[nexdrew]

@bcoe This is a lot more (unnecessary) overhead than just calling resetForNextParse(), meaning reset() will be called N+1 times (where N is the number of commands) for yargs.parse(msg, cb).

[bcoe]

see: #670

[kevupton]

@scriptdaemon I hadn't thought of this side-effect, but yes that totally works:

const yargs = require('./')
const parser = yargs
  .command('lunch-train [restaurant]', 'start lunch train', function () {}, function (argv) {
    console.log(argv.restaurant, argv.time)
    // api.scheduleLunch(argv.restaurant, moment(argv.time))
  })
  .help()

parser.parse('lunch-train', {restaurant: "Rudy's"}, function (err, argv, output) {
  if (output) bot.respond(output)
})```

@bcoe
When I am running console log inside my handler, it is being output to the console instead of being passed to the output variable, is this expected ?

[bcoe]

@kevupton yes, we don't actually override console in yargs, we just skip the steps that output to the console within yargs itself.

Instead of outputting to console.log perhaps you can populate a variable with text in the commands?