fix(commands): run a definition passed to the dispatcher as given

A name is looked up in the registry; a definition or Command() class is
built and run as the caller holds it, registered or not, its first name
serving only hooks and reporting.

Author: edusperoni

defining-commands.md

@@ -872,10 +872,11 @@ that holds an injector can call `CommandsService.runCommand` directly.
 
 ### Asking another command
 
-Both methods take the command's registered name, or — the typed way — the
-definition or `Command()` class it was registered from, whose first name is
-used: `runCommand(prepareCommandDefinition)` cannot go stale the way a string
-can.
+Both methods take a registered name, or — the typed way — a definition or
+`Command()` class. A name is looked up in the registry; a definition runs as
+given, whether or not it is registered, so `runCommand(prepareCommandDefinition)`
+runs exactly what you hold and cannot go stale the way a string can. Its first
+name still identifies it for hooks and reporting.
 
 `CommandsService.canExecuteCommand(command, args)` — or the
 `canExecuteCommand` convenience — asks a registered command whether it *could*

lib/common/contracts/commands-service.ts

@@ -22,8 +22,9 @@ export abstract class CommandsService {
 	 * failure throws instead of exiting, so a process that has to keep running
 	 * can catch it. Analytics do not fire: this is not a new CLI invocation.
 	 *
-	 * `command` is the registered name, or the definition or `Command()` class
-	 * it was registered from — the typed way to refer to a command.
+	 * `command` is a registered name, looked up in the registry, or a
+	 * definition or `Command()` class, which runs as given whether or not it is
+	 * registered — the typed way to refer to a command.
 	 */
 	abstract runCommand(
 		command: CommandReference,

lib/common/define-command.ts

@@ -763,27 +763,11 @@ export function toCommandDefinition(
 }
 
 /**
- * What a dispatcher accepts in place of a command name: the name itself, or
- * the definition or class it was registered from, whose first name is used.
+ * What a dispatcher accepts: a registered command's name, or a definition or
+ * class to run as given.
  */
 export type CommandReference = string | RegisterableCommand;
 
-export function commandNameOf(command: CommandReference): string {
-	if (typeof command === "string") {
-		return command;
-	}
-
-	const definition = toCommandDefinition(command);
-	if (!definition) {
-		throw new Error(
-			`${describeDefinition(command)} is neither a command name, a ` +
-				`defineCommand() definition nor a Command() class.`,
-		);
-	}
-
-	return Array.isArray(definition.name) ? definition.name[0] : definition.name;
-}
-
 /**
  * The class authoring form: sugar over defineCommand, not a second execution
  * path. The returned base carries a `definition` that reads the class it is

lib/common/services/commands-service.ts

@@ -11,7 +11,8 @@ import { injector } from "../yok";
 import { IExtensibilityService } from "../definitions/extensibility";
 import { IGoogleAnalyticsPageviewData } from "../definitions/google-analytics";
 import { CommandsService as CommandsServiceContract } from "../contracts/commands-service";
-import { CommandReference, commandNameOf } from "../define-command";
+import { CommandReference, toCommandDefinition } from "../define-command";
+import { createCommandFromDefinition } from "./command-definition-adapter";
 import {
 	ICommandParameter,
 	ICommand,
@@ -247,24 +248,28 @@ export class CommandsService
 	 * `checkConsent` may prompt on a terminal the caller has put in raw mode.
 	 */
 	public async runCommand(
-		command: CommandReference,
+		reference: CommandReference,
 		commandArguments: string[] = [],
 	): Promise<void> {
-		const commandName = commandNameOf(command);
+		// Known before the lookup, so a failure to resolve reports under the name
+		// the caller used.
+		let commandName = typeof reference === "string" ? reference : undefined;
 		this.inProcessDepth++;
 		try {
-			const command = this.$injector.resolveCommand(commandName);
-			if (!command) {
-				this.$errors.failWithHelp(
-					`Unknown command '${helpers.stringReplaceAll(commandName, "|", " ")}'.`,
-				);
-			}
+			const resolved = this.resolveReference(reference);
+			const command = resolved.command;
+			commandName = resolved.commandName;
 
 			this.commands.push({ commandName, commandArguments });
 			const restoreOptions = this.primeOptions(command);
 			try {
 				if (
-					!(await this.canExecuteResolvedCommand(commandName, commandArguments))
+					!(await this.canExecuteResolvedCommand(
+						commandName,
+						commandArguments,
+						undefined,
+						command,
+					))
 				) {
 					let commandWithArgs = commandName;
 					if (commandArguments && commandArguments.length) {
@@ -301,25 +306,21 @@ export class CommandsService
 	 * reuse another's precondition without importing its handlers.
 	 */
 	public async canExecuteCommand(
-		command: CommandReference,
+		reference: CommandReference,
 		commandArguments: string[] = [],
 	): Promise<boolean> {
-		const commandName = commandNameOf(command);
 		this.inProcessDepth++;
 		try {
-			const command = this.$injector.resolveCommand(commandName);
-			if (!command) {
-				this.$errors.failWithHelp(
-					`Unknown command '${helpers.stringReplaceAll(commandName, "|", " ")}'.`,
-				);
-			}
+			const { commandName, command } = this.resolveReference(reference);
 
 			this.commands.push({ commandName, commandArguments });
 			const restoreOptions = this.primeOptions(command);
 			try {
 				return await this.canExecuteResolvedCommand(
 					commandName,
 					commandArguments,
+					undefined,
+					command,
 				);
 			} finally {
 				restoreOptions();
@@ -352,6 +353,42 @@ export class CommandsService
 	 * and the host keeps reading the replacement long after the command is
 	 * done. An in-process dispatch has to put the parser back where it found it.
 	 */
+	/**
+	 * A name is looked up in the registry; a definition or class is run as the
+	 * caller holds it, registered or not, so what runs is what was referenced.
+	 * Its first name still identifies it for hooks and reporting.
+	 */
+	private resolveReference(reference: CommandReference): {
+		commandName: string;
+		command: ICommand;
+	} {
+		if (typeof reference === "string") {
+			const command = this.$injector.resolveCommand(reference);
+			if (!command) {
+				this.$errors.failWithHelp(
+					`Unknown command '${helpers.stringReplaceAll(reference, "|", " ")}'.`,
+				);
+			}
+
+			return { commandName: reference, command };
+		}
+
+		const definition = toCommandDefinition(reference);
+		if (!definition) {
+			throw new Error(
+				"Expected a command name, a defineCommand() definition or a " +
+					"Command() class to run.",
+			);
+		}
+
+		return {
+			commandName: Array.isArray(definition.name)
+				? definition.name[0]
+				: definition.name,
+			command: createCommandFromDefinition(definition, <any>this.$injector),
+		};
+	}
+
 	private primeOptions(command: ICommand): () => void {
 		if (command.isHierarchicalCommand) {
 			return () => undefined;
@@ -375,8 +412,9 @@ export class CommandsService
 		commandName: string,
 		commandArguments: string[],
 		isDynamicCommand?: boolean,
+		resolved?: ICommand,
 	): Promise<boolean> {
-		const command = this.$injector.resolveCommand(commandName);
+		const command = resolved || this.$injector.resolveCommand(commandName);
 		const beautifiedName = helpers.stringReplaceAll(commandName, "|", " ");
 		if (command) {
 			// Verify command is enabled

test/define-command.ts

@@ -1434,7 +1434,7 @@ describe("defineCommand", () => {
 			assert.isTrue(ran);
 		});
 
-		it("takes the definition or class in place of the name", async () => {
+		it("runs a definition or class as given, registered or not", async () => {
 			const testInjector = createInProcessInjector();
 			const runs: string[] = [];
 			const definition = defineCommand({
@@ -1453,22 +1453,31 @@ describe("defineCommand", () => {
 					runs.push("class");
 				}
 			}
-
+			// Registered under the same name as the definition, to show the
+			// definition wins over the lookup.
 			runInInjectionContext(testInjector, () => {
-				registerCommand(definition);
-				registerCommand(RefCommand);
+				registerCommand(
+					defineCommand({
+						name: "dctest-ref-primary",
+						arguments: "any",
+						run: () => {
+							runs.push("registered");
+						},
+					}),
+				);
 			});
 			const service = testInjector.get(CommandsServiceContract);
 
 			assert.isTrue(await service.canExecuteCommand(definition, ["ok"]));
 			assert.isFalse(await service.canExecuteCommand(definition, ["no"]));
 			await service.runCommand(definition, ["ok"]);
 			await service.runCommand(RefCommand);
-			assert.deepEqual(runs, ["definition", "class"]);
+			await service.runCommand("dctest-ref-primary");
+			assert.deepEqual(runs, ["definition", "class", "registered"]);
 
 			await assert.isRejected(
 				service.runCommand(<any>{ name: "not-a-definition" }),
-				/neither a command name/,
+				/Expected a command name/,
 			);
 		});