Emit new events beforeAction and beforeExec

[b-jsshapiro]

This is the promised pull request on issue #1197. Detailed explanation in the (only) commit message, but here's the summary:

1. Added Command._emitToTree(eventName, subEvent, args). Command._emitToTree() emits events on a command and on all of its parents, progressively expanding the set of emitted events so that sub-tree filtering can be performed even though the standard EventEmitter is being used. Would be nice if the core EventEmitter supported some filtering, but it doesn't.

2. Added invocations of Command._eventEmitter before actionHandler and execHandler calls, with the result that a callback occurs prior to execution. The eventNames are beforeAction and beforeEvent respectively.

There may be a better or cleaner approach, but this one at least has the merit of being clear, and didn't require broadly invasive changes.

[b-jsshapiro]

Oh yes. lint and test pass, per instructions.

[b-jsshapiro]

Closed the pull request by mistake while adding a comment. Sorry.

[shadowspawn]

I have worked on a similar problem in #1296 with allowing more control over the help. The pattern being used there is a pair of events, one of which is only sent to the command, and one which is sent to the tree (group) to allow more "global" behaviours.

Your example in your commit comment is:

At the typescript-types node, the emitted events are
   beforeAction:*
   beforeAction:typescript-types

At the schematool node, the emitted events are
   beforeAction:*
   beforeAction:schematool.*
   beforeAction:schematool.typescript-types

using a pair of events like in #1296 this simplifies to:

At the typescript-types node, the emitted events are
   preAction
   preGroupAction

At the schematool node, the emitted events are
   preGroupAction

Are there use cases you have in mind that need more events?

(On the naming, I narrowly went for "pre" over "before" for the help case, and using it here again to see how it feels in this scenario.)

[b-jsshapiro]

Hey, John, thanks for looking.

I like pre/post better than before/after. I used before in an attempt to follow one of the existing notification patterns.

FYI, my first proposal is already being used in near-production code. All it does is read a yaml file after options (which can override config values) are parsed, and I don't think I'd have been able to use commander without something to let me get in between the phases. Still easy enough to update here, but it provides at least some confirmation of practical value.

Regarding the hierarchy, yes there's a need, but on re-thinking there isn't a need for this much. Let me start with the CLI code to identify the problem:

program
  .name(path.basename(PkgName))
  .version(PkgVersion)
  .description(PkgDescription)
  .option('--no-doc-comments', 'exclude doc comments from output')
  .option('-c, --configfile <string>', 'configuration file path', 'schematool.yml')
  .option('--odir <string>', 'output directory name')
  .option('-o, --output <string>', 'output file name')
  .on('beforeAction:*', LoadConfig)
  ;

// Elsewhere:
program
  .command('typescript-types [schema...]')  // <- CHILD Command Node
  .description('Generate typescript typings for this schema')
  .action((schema) => {
    TypescriptTypes();
  })
  .on('beforeAction:*', (stuf) => { console.log('tst before* ts-types'); }) // this was testing
  .on('beforeAction:typescript-types', (stuf) => { console.log('tst before ts-types'); })
  ;

For my immediate application, I need that LoadConfig handler on the root to fire no matter what command is run. The problem is, the on() attaches the callback on the root-level Command object in the hierarchy, while the emit() call gets made on a child Command object. That's what led me to _emitToTree().

Here's how the rest evolved:

I realized why I wasn't seeing the event on the root-level Command object, kicked myself, and added _emitToTree so that it could be caught at the root. The ability to catch it at each level came for free and didn't seem like a bad thing, because tree-based filtering will be useful later for various things I had in mind. The embarrassing part of this is that I've written extensible command parsers of my own; the hierarchy of Command objects shouldn't have been a surprise.

Initially I just emitted ('beforeAction:commandName', { Command, cmdString, args }), but that hit a wall because EventEmitter's dispatcher doesn't provide any pattern matching. Sometimes I'll want to filter on the command name, so I decided (in haste and stupidly) to emit both 'beforeAction:commandName' and 'beforeAction:*', simulating the appearance of pattern matching. Then the hierarchy thing got my attention and I did the growing event list. Immediately saw the growing number of emits as a red flag, but it wasn't a bright enough red to get my attention in the wee hours.

Thinking about it after looking at your response, I think I would revise it to emit a single 'preAction' event at each Command node and change the payload value to be:

{ Command, 'leafCommandName', 'relative.path.to.leafCommandName', args }

Which can still be filtered in the handler if desired, still permits a subtree-based filter, doesn't suffer from a progressively increasing event count, and (considering your comment about #1269 ) subsumes the need for preGroupAction. Since the handlers might be async, and the node-relative path is changing as we go up the tree, I'd also update _emitToTree() to create a new payload object for each emit so that the emitted payload object doesn't get modified out from under a potentially deferred handler. Object spread is your friend.

Yeah, postAction and postExec are probably called for as well, though in some of those places there's an existing 'command' event being emitted already. Struck me as a little odd that those were post-events when I saw them, but I'm sure there was a reason.

Would you like a replacement pull request that puts all of this together? I'll add the post-events, but aside from that it's a smaller and simpler, which is usually a good sign. :-)

[shadowspawn]

Thanks for info, and a positive sign you are actually using it. I wasn't sure whether this was all in theory or actually solving a problem. 🙂

Since the handlers might be async

Async event handlers? My current thinking is fully supporting async event functions and pausing at the right points before proceeding requires much more machinery, and eventEmitter does nothing to help.

so that the emitted payload object doesn't get modified out from under a potentially deferred handler.

The opposite problem is that some use cases are going to want to modify the parse result and I am not sure the best way to achieve that. The most direct way is modifying the passed Command (which of course has problems with async again!) but it is not really set up for that.

though in some of those places there's an existing 'command' event being emitted already.

That was originally the way the commands were implemented by Commander, adding its own listeners. The implementation has changed to direct dispatching but the events were retained to avoid breaking existing client code which added additional listeners.

Would you like a replacement pull request that puts all of this together?

Thanks, but only if you are updating your own implementation anyway. I have plenty to think about.

[b-jsshapiro]

@shadowspan: updated per last exchange. Definitely simpler. Not entirely sure where the right place is for the postExec event, so I haven't dropped that in yet. My opinion would be to emit that after process close/error, but thought it best to confirm with you?

Also, I wonder if the process.error case shouldn't emit a distinct event with some error information. Any thoughts on what to call that event?

[shadowspawn]

@b-jsshapiro To be clear, I am finding this code and the discussion pretty interesting but may not merge this request. I don't want you to put work in assuming this is heading for a merge and be disappointed.

[shadowspawn]

Not entirely sure where the right place is for the postExec event, so I haven't dropped that in yet. My opinion would be to emit that after process close/error, but thought it best to confirm with you?

Short version: yes, that seems the most likely place and purpose.

Long version

(For postExec, I assume you are referring to running stand-alone executable subcommands using spawn. And "process close/error" are the events being listened for on the spawned process eventEmitter.)

The high level question is what problems does this help with, what is the user trying to achieve?

For telling that the subcommand has completed or terminated, yes could use a postExecutable emit when the sub-command process emits the close/error.

[shadowspawn]

Also, I wonder if the process.error case shouldn't emit a distinct event with some error information. Any thoughts on what to call that event?

Hmm, that triggers all sorts of thoughts.

I think the errors that come back from spawn probably don't have very rich error information in general. Going wider though...

The support that is available for modifying error handling and process termination is to use .exitOverride() which is passed a CommanderError including the error message displayed to the user (when relevant). I did actually wonder about emit-ing the error, but went with throwing an error for the much more familiar try/catch pattern for users. Commander is currently using events in a fairly light way as a convenience for some functionality rather than the scaffolding for the runtime.

EventEmitter does have an error event which is special. Listening for the event changes the runtime behaviour from triggering a throw to assuming the program has handled the error. We don't want the default behaviour for all errors to be a stack dump so not the right event to drop in, but interesting parallels.

[b-jsshapiro]

I don't currently have a use case for exec events. I put them in from a sense of symmetry. I don't have a use case for post events, and especially not given that you already have .exitOverride(). I wouldn't have added the post events, except that you mentioned them. Actually, I really don't like events. Given a free hand I would have added a .beforeCommand() callback.

As our exchange has proceeded, it turned out on my end that there was a reason to make a GetModuleConfig() call from the top of each command, so I can handle the config issue from there. Given which, I'm going to close out the pull request for lack of a use case.

From a design perspective, I'd encourage some form of .beforeCommand()-like thing. Even a cursory look at some basic UNIX or Windows commands will confirm that there are lots of cases of mutually exclusive options. In my opinion, it's cleaner to check those in one place than to replicate them in all of the relevant .option() callbacks. Distributing the logic rarely turns out to be maintainable.

Oh. While I have you, maybe consider moving the argv[0] pre-prep to parseOptions()? It's sitting in a kind of strange place at the moment, with the effect that parse() and parseOptions() can have different behavior, and it looks pretty trivial to relocate it. Let me know if a pull request on that would save you some work.

[shadowspawn]

maybe consider moving the argv[0] pre-prep to parseOptions()?

Not at this time. I think of parse as the routine doing the work of dealing with the runtime delivery details of command line arguments. parseOptions works with prepared arguments with just the "user" arguments.

[b-jsshapiro]

Fair enough. Then maybe add “parseOnly”? I don’t think you mean to, but you appear to be rejecting in sequence each approach that would readily enable a phase distinction. Cheers, Jonathan

…

On Fri, Aug 21, 2020 at 2:55 PM John Gee ***@***.***> wrote: maybe consider moving the argv[0] pre-prep to parseOptions()? Not at this time. I think of parse as the routine doing the work of dealing with the runtime delivery details of command line arguments. parseOptions works with prepared arguments with just the "user" arguments. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQPXV7UH6PUADAAB462XMFTSB3UMNANCNFSM4P626Q7A> .

[shadowspawn]

I don’t think you mean to, but you appear to be rejecting in sequence each
approach that would readily enable a phase distinction.

Ha, not the intention! 😄 Sorry I gave that impression..

To expand further, parseOptions is called at each level during recursive processing of the command line, for the program and for each subcommand. Adding argv[0] processing to it would only be relevant to the first call, and does not readily enable a phase distinction.

Emitting new events throughout the command line processing gives opportunity to inspect the state, but does not readily allow async code or modification of the command line processing or changes to the Command state.

I don't want to add code without a good understanding of what it offers and solves and whether it is sufficient.