Proposal for "run" handler at Api level about sywac HOT 8 OPEN

Addressing your concerns

Implementation of this feature would be very easy, but explaining it to a user might not be.

That hasn't stopped me before 🤣

But, seriously, I don't think this is a problem. It will make sense in certain scenarios, and we can describe it as a way to encapsulate your program's logic once argument parsing is complete.

Plus, assuming the top-level .run() handler is optional (I think it has to be) and we're still returning results/argv from .parse()/.parseAndExit(), then we're not precluding support for the old approach either, so CLI authors have the choice of using .run(handler).parseAndExit() or .parseAndExit().then(handler).

The issue with supporting "run" ... is that it conflicts with the command's run

I don't think they really conflict - you have the same issue today if you have logic that uses argv after .parseAndExist() resolves. Again, it's completely optional.

If I ran myapp start, which run function in this program should I expect to execute?

Just like you should expect both the command handler AND any logic that comes after .parseAndExit() to run when using sywac today, I think you should expect both the command handler and the top-level handler to run, in that order. You can always check some state in the top-level handler to see if your command ran and, if it did, just do a no-op.

My concerns

The one question I have is what arguments should be passed to the top-level run handler.

If we want to maintain consistency with a command run handler, then it should be (argv, context), but I have a couple issues with that:

1. I don't really want to pass it the context, since that might imply that manipulating it (e.g. context.cliMessage()) will affect the overall parsing result when, in fact, overall parsing is complete by the time the top-level run handler is called. This is why .parse() resolves to context.toResult() instead of context.

2. If I use .parse() instead of .parseAndExit(), then CLI authors expect a result object (which wraps argv) instead of just the argv object. So just providing argv to the top-level run handler is probably insufficient.

Because of these issues, I think we should consider one or more of the following:

1. Use an api method name other .run(), to avoid confusion with/expectations of a command run handler. Perhaps something like .afterParse() or .handleResult()?

2. Possibly provide two api methods that correspond to using either .parse() or .parseAndExit(), where each handler should expect to receive as an argument the same object that would be returned/resolved from the respective parse method, namely result (when using .parse()) or argv (when using .parseAndExit()). If we take this approach, then we'd probably need to execute both types of top-level run handlers if both are given/defined.

Personally I think we should do number 1 and not number 2, since number 2 could be confusing.

With this approach, we should probably just pass the result object to the top-level run handler and expect the consumer to use result.argv if it only cares about the parsed argv.

The end result would look something like this:

#!/usr/bin/env node

const cli = require('sywac')
  /* ... options, commands, etc ... */
  .afterParse(result => {
    const myLib = require('./index')
    return myLib(result.argv)
  })

module.exports = cli

if (require.main === module) cli.parseAndExit()

which would allow you to write tests that require('./cli') and call .parse() and get approximately the same result as actually executing the bin in a separate process.

What do you think?

from sywac.

Your reasoning seems sound, but I am worried about point (1).

In a Command run handler, you are technically mid-parse, which means you have the flexibility to throw an Error or call context.cliMessage, either will work, depending on your needs.

This flexibility is part of the point of the "top level run handler", in my eyes - more flexible than parse().then().

I guess I would want the run handler to take (argv, context) and to allow context.cliMessage - implying probably that it gets called in a very specific spot (near the end of _parse perhaps).

from sywac.

This flexibility is part of the point of the "top level run handler", in my eyes ... I guess I would want the run handler to take (argv, context) and to allow context.cliMessage

Oh ok, interesting. I wasn't thinking of it that way, but that makes sense.

In that case, I assume we would just call it .run(handler) (like you originally suggested).

What do you think about my argument to execute both a command handler and the top-level run handler when both are defined/given? Are you ok with that?

from sywac.

I think I am. It means the command's run handler and your setup run handler behave slightly differently, but that is something we can document, for sure.

To get more detailed, here's a test case that I think covers all the bases:

// pizza.js
require('sywac')
  .check((argv, context) => console.log('[check] level 1'))
  .run((argv, context) => console.log('[run] level 1'))
  .command('order', {
    setup: sywac => {
      sywac
        .check((argv, context) => console.log('[check] level 2'))
        .run((argv, context) => console.log('[run] level 2'))
        .command('cancel', {
          setup: sywac => {
            sywac
              .check((argv, context) => console.log('[check] level 3'))
              .run((argv, context) => console.log('[run] level 3'))
          },
          run: (argv, context) => console.log('[run] order cancel')
        })
    },
    run: (argv, context) => console.log('[run] order')
  })
  .parseAndExit()

$ pizza order cancel
[check] level 1
[check] level 2
[check] level 3
[run] order cancel
[run] level 3
[run] level 2
[run] level 1

$ pizza order
[check] level 1
[check] level 2
[run] order
[run] level 2
[run] level 1

Although it's not necessarily intuitive for a first-time user of Sywac that nested run handlers execute in LIFO order, I think this is the way it has to be -- .run() needs to be the last that is executed before an Api "pops off" the stack, because it's a replacement for .parse().then().

I haven't even looked at how messy it would be to implement, but my take on the requirements is:

1. Nested checks occur in FIFO order (like today).
2. Command "run" handler executes next, and only the command run handler actually selected (like today).
3. Nested run handlers occur in LIFO order (new).

I actually don't know if it would be that common to mix and match run handlers and Command run handlers like this, I just think this approach makes everything work together in the most understandable way, after the initial learning curve.

from sywac.

Next-day thoughts:

I might have given up too soon on this order:

$ pizza order cancel
[check] level 1
[run] level 1
[check] level 2
[run] level 2
[check] level 3
[run] level 3
[run] order cancel

• PRO: More intuitive, behaves "more like written" than my 1st approach.
• CON: Less powerful/flexible. (You can already use check() to provide nested levels of setup behavior, so run() is almost superfluous when used in this way.)
• CON: Type post-parsing (where commands run) would need to happen after all of the checks and runs have already completed, which potentially hamstrings "post parse" functionality. (That is: if you invent a new type that uses postParse in some unique way, it's severely limited because by the time it runs everything is already over).

So anyway, I thought I'd walk back my "it HAS to be this way" statement. But I still think the last-in-first-out run() approach is more powerful overall.

from sywac.

Thanks for that exercise.

I'm not sure your 2nd option (the next-day thought) is viable, because I think we have to (or at least should) run the command handler before running the Api run handler. I don't think it makes sense to try to run the Api handler between the parse and postParse lifecycle methods of argument Types; I think it has to run after postParse (which, as you pointed out, is when the command handler is run).

You can already use check() to provide nested levels of setup behavior, so run() is almost superfluous when used in this way.

I think this is a good point that gives weight to the argument for executing the Api run handler last (after postParse) - check and run have two different purposes, and run should not be used for something that can be accomplished by check.

I'm leaning toward a third option: explicitly disable support for run in child Api instances

Assuming sywac is used as intended, we can tell when an Api instance is a child or not. For children, we can simply ignore (or error) when .run() is called.

IMO, when a CLI program (or chatbot parser) is run, the goal should be to determine which individual/single handler to execute (regardless of "level") and then execute it with the given/parsed arguments. The concept of "kicking off" more than one handler per command/message seems unintended and possibly inappropriate to me. The only reason child Api instances exist at all is to support commands. The fact that sywac uses a child Api instance to organize arguments and options specific to each command is just an implementation detail - the side effects of that implementation detail should not necessarily be construed as an intended feature.

So, the way I'm thinking about it, the Api run handler should typically be in an "either-or" relationship with command handlers - either you use/run one or you use/run the other, but not both together. If they are used both together, then I think that's only so the top-level Api handler can run as a backup/default when no command handler was run. (And we can even implement it so that we guarantee only one handler runs - either a command handler or the top-level Api handler - per .parse() call.)

I should mention that whatever you're trying to achieve in your CLI with a nested Api run handler should probably be accomplished via a default command (which are command-level specific), since that is the purpose of default commands.

For these reasons, I don't think we need to support nested levels of Api run handlers.

What do you think? Is there a valid use-case for wanting to run multiple run handlers (at different levels)?

from sywac.

What do you think? Is there a valid use-case for wanting to run multiple run handlers (at different levels)?

Nope! I like your suggestion, as I think we can state it really plainly: in any sywac execution, only one .run() handler is ever called. Once you know that, it's relatively easy to piece together which run handler will execute.

Do we need errors? I don't know that we need them, but it might save users from some head-scratching. There's probably two places we could error that might be helpful:

1. On an Api .run() defined on an inner Api: Instead of creating a .run() handler in your setup(), add your .run() handler directly to your command definition.

2. On the outer Api, if any commands are defined: When using commands, instead of a .run() handler, add a default command ('*') and give it a run handler.

from sywac.

Works for me! Let's give it a shot.

from sywac.