createCommand factory routine

[shadowspawn]

Pull Request

Related: #1002 #1185

Problem

• some people prefer factory routine to using new
• awkward to subclass Command
• awkward to customise Command (without using subclass)

Solution

Add a factory routine, which can be overridden.

[vonagam]

createCommand in types should return Command not this, since it returns a new command instance.

Initially i thought that this is a less flexible solution then construct, since with construct option you can do these things:

class CustomCommand extends Command {
  command(name, opts) { 
    const construct = (name) => {
      if (opts.customOpts) {
        return new CustomCommand(name);
      } else {
        return new Command(name);
      }
    };
    return super.command(name, { construct, ...opts });
  }
}

class CustomCommand extends Command {
  constructor(name, customArg) {
    super(name);
    // custom stuff with custom arg
  }

  command(name, opts) { 
    const construct = (name) => {
      return new CustomCommand(name, opts.customArg);
    };
    return super.command(name, { construct, ...opts });
  }
}

But you can achieve same results by assigning createCommand before calling super in command and since it is a sync operation construct and createCommand are functionally the same, while createCommand is more clear for general use case.

So thumb up from me, i will close original PR in favour of this one.

[shadowspawn]

createCommand in types should return Command not this, since it returns a new command instance.

this represents the subtype of the containing class, rather than indicating it is the this object as such, although for a fluent function they are the same type! So createCommand in MyCommand should return an object of type MyCommand?

https://www.typescriptlang.org/docs/handbook/advanced-types.html#polymorphic-this-types

With this pattern, I now think following signature should also return this as the expected behaviour?

    command(nameAndArgs: string, opts?: CommandOptions): Command;

[vonagam]

In provided link all functions return this so automatically even without declaration they will return type this.

I would recommend to stick to the way typescript automatically infers types. For example this code is invalid:

class Custom {
    create(): this {
        return new Custom();
    }
}

The error is "Type 'Custom' is not assignable to type 'this'.", you can check it here - https://www.typescriptlang.org/play/index.html

Plus not all subclasses can override createCommand. But if they do, they are allowed to change type to subclass like this:

class Command {
    create() {
        return new Command();
    }
}

class Custom extends Command {
    create() {
        return new Custom();
    }
}

const command = new Custom();
const nested = command.create(); 

nested is recognised here as Custom type, not Command, but if there is no redefinition of create() in Custom then nested is base Command type as it should be.

There is a small problem with command() return type, so if you want to change createCommand you will also need to redefine command() too with needed subclass type as a return type and simple call to super:

class Command {
  command() {
    return new Command();
  }
}

class CustomCommand extends Command {
  command(): CustomCommand {
    return super.command();    
  } 
}

So just to be clear, if you will keep it this instead of Command, this code (which you use in tests) will be invalid:

class CustomCommand extends Command {
    createCommand(name: string) {
        return new CustomCommand(name);
    }
}

So there is no option to keep this as a return type.

[shadowspawn]

Thanks @vonagam , I didn't do much experimenting in TypeScript and made wrong assumptions about this as a return type.

I like how the pattern is working from JavaScript with the things I tried, but less happy from TypeScript where the return type from .command() is still a complication, as you pointed out.

The TypeScript issues got me looking wider. I am still thinking about intended patterns, and considerations like whether factory should be static, and should it create instance of subclass automatically, and in TypeScript should .command() return be typed to subclass. Whether focus is making subclassing easier, or improving extending in general.

Example of more full featured approach for subclassing support:

• Polymorphic "this" for static members microsoft/TypeScript#5863 (comment)

[vonagam]

No prob, you got me too, had to double check that i myself understand this right.

Also, someone might want a subclass to not return an instance of that subclass for command and in constructor for that subclass there might be some other arguments than simply name. So if nothing is redefined than createdCommand and command should return Command.

Here is a possible solution if you want to avoid redefining command():

class Command {
    command(): ReturnType< this[ 'createCommand' ] > {
        return this.createCommand() as any;
    }

    createCommand() {
        return new Command();
    }
}

class CustomCommandA extends Command {
}

class CustomCommandB extends Command {
    createCommand() {
        return new CustomCommandB;
    }
}

const a = new CustomCommandA();
const aSub = a.command(); // type Command

const b = new CustomCommandB();
const bSub = b.command(); // type CustomCommandB

So make ReturnType< this[ 'createCommand' ] > return type for command().

[shadowspawn]

Nice! That is a much less restrictive approach than locking down the subclass as the return type, and covers the use cases I had in mind so far.

[shadowspawn]

I am pretty happy with how this has turned out. Ready for review.

The possible downsides I can see are:

1. people mistakenly using .createCommand() instead of .command(). I have added comments in JSDoc to try and mitigate this.
2. people expecting Commander to be more friendly to class overrides. Not planning to address this, the event listeners are still the explicit extension mechanism.

[shadowspawn]

@vonagam Are you interested in doing a review?

(No worries if you have moved on or busy, and thanks for all the good feedback!)

[vonagam]

Sorry, didn't know that you were waiting for my review, thought it was about that other main collaborator.

Looks good otherwise, so that's why i haven't said anything. (Strange that the return type wasn't picked by linter.)

[shadowspawn]

Thanks @vonagam , I wasn't explicitly waiting on you but I expect @abetomo was giving you the opportunity as an interested and knowledgable contributor. :-)

I suspect we aren't running lint on the TypeScript. I'll look into that as we are making regular changes now.

Good feedback, thanks. And thanks again for the TypeScript advice earlier.

[abetomo]

I thank you for your dedication to the project.

[shadowspawn]

Thanks for reviews @vonagam and @abetomo

I'll do another prerelease soon. I think this might be last code change for 5.0.