Add support for "local" and global help:header, help:body and help:footer #1267

JCQuintas · 2020-05-27T21:32:12Z

Pull Request

Problem

This adds the possibility to customize the header, body and footer of the help command. In theory this will also allow for the "suppression" of the help command if you supply an empty function to the "help:body" listener.

It solves part of the problems listed in #1225

Solution

There are two solutions on this PR:

Local or Scoped Help Listeners

For "local" or "scoped" help, we now have three listenable events. Which are called in the following order:

help:header
help:body
help:footer

When you add a listener to help:body, the default help text won't be written to the stdout.

All emitters send their scoped this to the listeners as a parameter.

program
  .on('help:header', () => {
    console.log('Custom Header\n');
  })
  .on('help:body', (instace) => {
    console.log('------------');
    console.log(instace.helpInformation().replace(/\n$/, '\n------------\n'));
  })
  .on('help:footer', () => {
    console.log(`Commander - ${new Date().getFullYear()}`);
  })

// Custom Header
//
// ------------
// Usage: help-listeners [options]
//
// Options:
//   -V, --version  output the version number
//   -b, --bar      enable some bar
//   -h, --help     display help for command
// ------------
//
// Commander - 2020

Global Help Listeners

I would like more input on this, as I believe right now it might have side effects when using multiple commands? But I'm not sure and this would require testing.

The global listeners behave similarly to local listeners and use the same keys, but affect all the help calls. Root and children commands. Local commands will still be called and the order is:

global help:header
local help:header
global help:body
local help:body
local help:footer
global help:footer

Typings, tests and documentation

ChangeLog

shadowspawn · 2020-05-29T06:15:26Z

Is this something you want to use yourself, or just coding it?
The global support is a challenge. It is harder than doing the local ones, but in practice is something people are quite likely to want to "brand" their whole program, so I think a good idea you have attempted it. Some initial thoughts.

The local listeners could be added manually everywhere with customisation via .createCommand, but that would be a barrier to usage, so more a work-around than a solution.

I don't think using a true global is the way to go for holding the listeners. The problem is if different packages in a program use Commander internally, they interact via any globals. Commander did start out as global only and have this problem, but we are now encouraging the use of new Command() to make things independent.

A possible approach would be differently name events which are emitted by walking the command tree at runtime, following the parent links. Say help:global-header or global-help:header. Still thinking about the terminology and the approach, so this is a suggestion and not a recommendation. The extra events would actually be in the tree hierarchy so not truely global, but would affect all subcommands.

shadowspawn · 2020-05-29T06:15:51Z

(And thanks for the contribution!)

JCQuintas · 2020-05-29T14:53:05Z

Hi, I would like this for my own, yes. Overriding every single help to add a simple header gets old fast.

And yes, I saw your "help issues compilation" and thought the help:header/parts was a simple enough to implement idea. I added the global ones because I wanted it and though it would be nice, but I understand the issues it would cause. I agree that a traversable tree will be a better option, but I wanted to gather input from the collaborators on it before I did anything drastic.

With that said, I think your comments on the "local" listeners wasn't clear on your comment, do you think it can work the way I implemented? I can always remove the global listeners if the local ones are "go". 😄

shadowspawn · 2020-05-30T04:07:51Z

With that said, I think your comments on the "local" listeners wasn't clear on your comment, do you think it can work the way I implemented?

Yes in general, that was what I had in mind, but l have a lot of suggestions on the details. You don't need to act on them at this stage unless you wish, consider them discussion points. Giving me an opportunity to develop the skeleton idea you started from.

shadowspawn · 2020-05-30T04:59:40Z

typings/index.d.ts

+  | 'help:header'
+  | 'help:body'
+  | 'help:footer'
+  | (string & {})


This does indeed seem to work in Visual Studio Code. Do you have any documentation or a source for what (string & {}) is doing to the type? An intersection of a string and an object with no properties?

I went to hunt for some sort of explanation on this but couldn't find any. So I'll try to explain what I believe happens.

An empty interface {} is almost like an "any", except it doesn't allow the undefined | null, we can see it if you parse the code below on an editor:

interface Empty {} const n:Empty = 0 const s:Empty = 'a' const a:Empty = [] const o:Empty = {} const t:Empty = true const f:Empty = false const sy:Empty = Symbol() const u:Empty = undefined //error const nu:Empty = null //error

So the typescript intersection (string & {}) would mean it has to be a string but can't be null or undefined.

When you declare const foo: 'a' | 'b' | string = '' typescript seems to coerce all the values into the string type. But when using (string & {}), it doesn't, because it is not expecting a "true" string.

The two drawbacks I can find or thing of are:

In some typeguard functions like the one below, it will throw an error, though it is easily bypass-able.

function foo(arg: 'a' | 'b' | (string & {})): 'a' | undefined { if (arg === 'a') { return arg; // Type '(string & {}) | "a"' is not assignable to type '"a"' //return arg as 'a'; // this works } return }

The other drawback might be that this could be patched in the future, since I couldn't find any documentation or explanation for it. Though I suspect if patched, it would behave just like a string type instead. But who knows...

An alternative to this would be to overload the "on" function with

on(event: 'help:header' | 'help:body' | 'help:footer' | symbol, listener: (...args: any[]) => void): this; on(event: string, listener: (...args: any[]) => void): this;

shadowspawn · 2020-05-30T05:04:48Z

examples/help-listeners

+  .on('help:header', () => {
+    console.log('Custom Header\n');
+  })
+  .on('help:body', (instance) => {
+    console.log('------------');
+    console.log(instance.helpInformation().replace(/\n$/, '\n------------\n'));
+  })
+  .on('help:footer', () => {
+    console.log(`Commander - ${new Date().getFullYear()}`);
+  })


The replace and \n are a bit confusing here. I eventually recalled that helpInformation() has a trailing return for legacy reasons because it is used with process.stdout.write. I suggest a simpler more symmetrical pattern like:

.on('help:header', () => { console.log('Custom Header\n'); }) .on('help:body', (instance) => { console.log('------------'); process.stdout.write(instance.helpInformation()); // has trailing eol console.log('------------'); }) .on('help:footer', () => { console.log(`\nCommander - ${new Date().getFullYear()}`); })

yeah, what you suggested was close to my first approach I believe, but I wanted the output to be neat. But indeed it is much better to read.

shadowspawn · 2020-05-30T05:11:05Z

Event parameter. I had been thinking we would pass the helpInformation (or even cbOutput) as a parameter to the help:body event, but I do like the symmetry of passing the command object to all the help listeners and using the same parameters. So preferring what you did at the moment.

shadowspawn · 2020-05-30T05:14:45Z

index.js

+    this._helpHeader = 'help:header';
+    this._helpBody = 'help:body';
+    this._helpFooter = 'help:footer';


These can be used as literals rather than having a property for them. You were probably copying the pattern for _helpLongFlag but it is a property because it can be changed by the user.

shadowspawn · 2020-05-30T05:20:58Z

index.js

+      globalEventEmitter.emit(this._helpBody, this);
+      this.emit(this._helpBody, this);


I think only one of the body events should get emitted, the local one in preference to the global one.

Yeah I think it might be better too, I was in doubt of what to do there, thanks :)

shadowspawn · 2020-05-30T05:31:15Z

index.js

+    const globalBodyListeners = globalEventEmitter.listeners(this._helpBody);
+    const bodyListeners = this.listeners(this._helpBody);


I have been moving to a different pattern for testing listeners, since we don't call the listener directly we do not need the list. Just check the count. An example of pattern, not a direct code replacement:

if (this.listenerCount('help:body')) { this.emit('help:body', this); } else { process.stdout.write(cbOutput); }

How is this.litenerCount supposed to know how many listeners there are? Or is this just an indirect call to this.listeners('whatever').length?

listenerCount is an EventEmitter method: https://nodejs.org/api/events.html#events_emitter_listenercount_eventname

Oops. Yeah is missed that. 😅

shadowspawn · 2020-05-30T05:32:50Z

index.js

@@ -1530,12 +1534,23 @@ class Command extends EventEmitter {
        return passthru;
      };
    }
+    globalEventEmitter.emit(this._helpHeader, this);
+    this.emit(this._helpHeader, this);
    const cbOutput = cb(this.helpInformation());


In theory we should try and include or make available cb in the processing of the listener too, but that callback is a pattern I would like to deprecate. Instead, the processing of the callback can shift into the non-override body block since no need to call if not being displayed.

shadowspawn · 2020-05-30T05:49:25Z

That is probably enough comments from me for today!

shadowspawn · 2020-06-01T00:41:04Z

Prompted by this PR, I had a rethink of the event names to include the extra level, as I don't think the page layout terms work as well for that (i.e. header, body, footer). This is my favourite so far:

1. globalPreHelp
2. preHelp
3. help | globalHelp
4. postHelp
5. globalPostHelp

JCQuintas · 2020-06-01T12:34:38Z

Event parameter. I had been thinking we would pass the helpInformation (or even cbOutput) as a parameter to the help:body event, but I do like the symmetry of passing the command object to all the help listeners and using the same parameters. So preferring what you did at the moment.

I decided to pass "this" to the listener so you would be able to check all the commands/parsed arguments or anything really if you wanted to customize the help while using an arrow function. In theory the "local" listeners already pass "this" to function () {}, but it doesn't work with () => {}

JCQuintas · 2020-06-01T12:39:07Z

Prompted by this PR, I had a rethink of the event names to include the extra level, as I don't think the page layout terms work as well for that (i.e. header, body, footer). This is my favourite so far:

1. globalPreHelp
2. preHelp
3. help | globalHelp
4. postHelp
5. globalPostHelp

I think it is better than header/footer, but I prefer the : as divider, though this is mostly a cosmetic change, so it is ultimately your choice. :P

1. pre:help:global
2. pre:help
3. help | help:global
4. post:help
5. post:help:global

JCQuintas · 2020-06-06T22:14:36Z

I've been looking more through the code to see how to improve this PR. And I had not notice that we always have access to this.parent, so traversing the tree is easy. Now your suggestion of a listener tree is clearer to me. :)

So the idea would be to search for every globalPreHelp in the parent tree, but trigger only the one that is closest to root? This might be a bit misleading as they would not be trully global.

- command('foo')
  - command('bar').addListener('globalHelp')
    - command('bas')
  - command('xoo')
    - command('yoo')

foo            # not
foo bar        # trigger
foo bar bas    # trigger
foo xoo        # not
foo xoo yoo    # not

What are the advantages of this vs always triggering the global listener only the "root" command?

shadowspawn · 2020-06-06T23:31:44Z

So the idea would be to search for every globalPreHelp in the parent tree, but trigger only the one that is closest to root? This might be a bit misleading as they would not be trully global.

All of the globalPreHelp would be called. Only the closest globalHelp would be called.

Yes, the listener is only "global" from that point in the tree, and not truly global. I am not entirely happy with it myself! I think the most command usage will be adding it to the root command, but I would prefer more accurate wording which is still easy to understand.

The qualifying word could go on either the "global" flavour or the "local" flavour.

Alternative suggestions or examples welcome!

What are the advantages of this vs always triggering the global listener only the "root" command?

This does allow a custom behaviour for a subcommand and all its subcommands (say to add a description of shared options)

shadowspawn · 2020-06-07T01:05:19Z

I spent some time wondering about other words and looking for other uses. I came across "group" to describe a command and its subcommands. So could look like:

preGroupHelp
preHelp
help | groupHelp
postHelp
postGroupHelp

shadowspawn · 2020-06-10T05:50:51Z

I'll mention the other thing I want to consider at the same time as part of adding new events. When the help is being displayed due to a usage error, it should be displayed on stderr rather than stdout. This has only come up once I am aware of in #997, so not much demand.

To make it easy for implementers I am thinking probably want easy access to both a console.log() equivalent and a process.stdout.write() equivalent. Interested if write is actually important to implementers, but as we know from this PR the way helpInformation is currently constructed with a trailing \n makes only offering a log a (small) complication.

Since most people will only want one or the other, I was thinking they could go in a "context" object. I have looked for the stdout/stderr problem being solved elsewhere and not found examples. I might be missing a simpler solution!

shadowspawn · 2020-06-13T06:56:30Z

Trying out the context pattern idea, parameter could look like:

const helpContext = {
  'command': program,
  'write': (...args) => process.stdout.write(...args),
  'log': (...args) => console.log(...args),
}

And be used like:

  .on('help', (context) => {
    context.write(context.command.helpInformation());
  })

JCQuintas · 2020-06-13T07:07:48Z

Why would someone use context.write if they have access to their own loggers? Or is this the internal implementation?

shadowspawn · 2020-06-13T07:13:35Z

context.write might call process.stdout.write or process.stderr.write depending on why the help is being displayed. This is especially relevant when someone is letting Commander display the normal help and just adding a banner or footer, so that everything goes to the same stream.

shadowspawn · 2020-06-15T02:40:48Z

Ok if I work on this too @JCQuintas , or would you prefer I leave it to you for now?

JCQuintas · 2020-06-15T07:11:53Z

Sure, go ahead. :)

shadowspawn · 2020-06-20T00:15:03Z

Experimenting on branch: https://github.com/shadowspawn/commander.js/tree/feature/on-help

shadowspawn · 2020-08-02T04:36:34Z

I'll close this in favour of #1296, with especially the global listener idea here inspiring the current form of that PR.

I am intrigued by the TypeScript trick to get Intellisense suggesting event strings in Visual Studio Code, but without some backing references it feels a bit too hacky.

Thank you for your contributions.

JCQuintas added 8 commits May 27, 2020 20:56

implement header, body and footer help emiters

f34c7d9

add examples

4a32a73

fix example and code

5e09664

add typings

0f57932

add tests and fix typings

69cdff6

fix linting issues

e69b077

fix ts linting

a3bf3d2

fix typos

51dbcab

shadowspawn reviewed May 30, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

shadowspawn reviewed May 30, 2020

View reviewed changes

shadowspawn mentioned this pull request Jun 1, 2020

Question: Is it possible to have a splash screen? #1272

Closed

shadowspawn mentioned this pull request Jul 4, 2020

Expand help customisation with .addHelpText #1296

Merged

shadowspawn closed this Aug 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for "local" and global help:header, help:body and help:footer #1267

Add support for "local" and global help:header, help:body and help:footer #1267

JCQuintas commented May 27, 2020

shadowspawn commented May 29, 2020

shadowspawn commented May 29, 2020

JCQuintas commented May 29, 2020

shadowspawn commented May 30, 2020 •

edited

shadowspawn May 30, 2020

JCQuintas Jun 1, 2020

JCQuintas Jun 1, 2020

shadowspawn May 30, 2020

JCQuintas Jun 1, 2020

shadowspawn commented May 30, 2020 •

edited

shadowspawn May 30, 2020

This comment has been minimized.

shadowspawn May 30, 2020 •

edited

JCQuintas Jun 1, 2020

shadowspawn May 30, 2020 •

edited

JCQuintas Jun 1, 2020

shadowspawn Jun 1, 2020

JCQuintas Jun 1, 2020

shadowspawn May 30, 2020

shadowspawn commented May 30, 2020

shadowspawn commented Jun 1, 2020 •

edited

JCQuintas commented Jun 1, 2020

JCQuintas commented Jun 1, 2020

JCQuintas commented Jun 6, 2020

shadowspawn commented Jun 6, 2020

shadowspawn commented Jun 7, 2020 •

edited

shadowspawn commented Jun 10, 2020

shadowspawn commented Jun 13, 2020

JCQuintas commented Jun 13, 2020

shadowspawn commented Jun 13, 2020

shadowspawn commented Jun 15, 2020

JCQuintas commented Jun 15, 2020

shadowspawn commented Jun 20, 2020

shadowspawn commented Aug 2, 2020

		globalEventEmitter.emit(this._helpBody, this);
		this.emit(this._helpBody, this);

		const globalBodyListeners = globalEventEmitter.listeners(this._helpBody);
		const bodyListeners = this.listeners(this._helpBody);

Add support for "local" and global help:header, help:body and help:footer #1267

Add support for "local" and global help:header, help:body and help:footer #1267

Conversation

JCQuintas commented May 27, 2020

Pull Request

Problem

Solution

Local or Scoped Help Listeners

Global Help Listeners

Typings, tests and documentation

ChangeLog

shadowspawn commented May 29, 2020

shadowspawn commented May 29, 2020

JCQuintas commented May 29, 2020

shadowspawn commented May 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadowspawn commented May 30, 2020 • edited

Choose a reason for hiding this comment

This comment has been minimized.

shadowspawn May 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadowspawn May 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadowspawn commented May 30, 2020

shadowspawn commented Jun 1, 2020 • edited

JCQuintas commented Jun 1, 2020

JCQuintas commented Jun 1, 2020

JCQuintas commented Jun 6, 2020

shadowspawn commented Jun 6, 2020

shadowspawn commented Jun 7, 2020 • edited

shadowspawn commented Jun 10, 2020

shadowspawn commented Jun 13, 2020

JCQuintas commented Jun 13, 2020

shadowspawn commented Jun 13, 2020

shadowspawn commented Jun 15, 2020

JCQuintas commented Jun 15, 2020

shadowspawn commented Jun 20, 2020

shadowspawn commented Aug 2, 2020

shadowspawn commented May 30, 2020 •

edited

shadowspawn commented May 30, 2020 •

edited

shadowspawn May 30, 2020 •

edited

shadowspawn May 30, 2020 •

edited

shadowspawn commented Jun 1, 2020 •

edited

shadowspawn commented Jun 7, 2020 •

edited