fix(commands): infer the class form's result type and validate its meta eagerly

Command() took a third type parameter for the run result that nothing
could infer, so a class whose run returned a value only compiled once
the author restated the name and the option schema as type arguments.
CommandBase now declares run as returning unknown and types postRun off
the subclass's own run through the polymorphic this, so the result is
inferred and CommandClass has two parameters.

The meta passed to Command() was only checked on the first read of the
class's definition, which for a registered built-in is the first
resolution in a user's terminal. It is now validated at the Command()
call itself: unknown fields, bad option or argument specs, and handlers
passed as meta rather than declared as methods all throw there. Only a
missing run method still waits for the definition, and that message no
longer leaks the factory's local class name.

Author: edusperoni

defining-commands.md

@@ -59,7 +59,9 @@ A definition is checked at the moment `defineCommand` is called, not when the
 command eventually runs. A misspelled field, a missing `run`, an option
 declared with something other than the four helpers, an `arguments` value
 outside `"none" | "any"` — each throws immediately, naming the command and the
-accepted form:
+accepted form. The class form's meta is checked the same way at the
+`Command({ ... })` call, which also rejects handlers passed there; only a
+missing `run` method waits until the definition is first read:
 
 ```
 Invalid command definition for 'widget|add': unknown field(s) 'handler'; a
@@ -563,7 +565,8 @@ export class PlatformCleanCommand extends Command({
 `arguments`, `allowUnknownOptions`, `disableAnalytics` and `enableHooks`. The
 handlers are methods instead — `run` is required, and `canExecute`, `postRun`
 and `shortcuts` are optional, each with the same meaning and the same ordering
-as the fields of the same name. `postRun(result)` receives what `run` returned;
+as the fields of the same name. The result type is inferred from `run`, and
+`postRun(result)` receives it, awaited, with no type argument to restate;
 `shortcuts()` returns the same table `shortcuts(ctx, setup)` does. A method the
 class does not declare is left out of the definition entirely, so a class
 without `postRun` gets no `postCommandAction`, exactly as an object without one

lib/commands/create-project.ts

@@ -368,11 +368,7 @@ async function interactiveFlavorAndTemplateSelection(
 	);
 }
 
-export class CreateProjectCommand extends Command<
-	"create",
-	typeof createProjectCommandOptions,
-	ICreateProjectData
->({
+export class CreateProjectCommand extends Command({
 	name: "create",
 	description: "Creates a new NativeScript project.",
 	options: createProjectCommandOptions,

lib/common/define-command.ts

@@ -117,8 +117,8 @@ export interface CommandContext<TSchema extends CommandOptionsSchema = {}> {
 	/** Current value of every option declared in the schema, and nothing else. */
 	options: CommandOptionValues<TSchema>;
 	/**
-	 * The injector the command was registered against. `inject()` stops working
-	 * after the first `await`; this is the supported late lookup.
+	 * This invocation's injector, the one `inject()` resolves against before the
+	 * first `await`; after it, `inject()` stops working and this is the lookup.
 	 */
 	injector: Injector;
 	/** Fails the command with `message` and the usage help suggestion. */
@@ -539,6 +539,49 @@ const validateDefinition = (definition: any): void => {
 	}
 };
 
+const HANDLER_FIELDS = ["setup", "canExecute", "run", "postRun", "shortcuts"];
+
+const META_FIELDS = DEFINITION_FIELDS.filter(
+	(field) => HANDLER_FIELDS.indexOf(field) === -1,
+);
+
+/**
+ * Everything defineCommand checks except the handlers, which the class form
+ * only has once the subclass is declared, after Command() has returned.
+ */
+const validateMeta = (meta: any): void => {
+	if (!isPlainObject(meta)) {
+		invalid(meta, "Command() expects an object");
+	}
+
+	const fields = Object.keys(meta);
+	const handlers = fields.filter(
+		(field) => HANDLER_FIELDS.indexOf(field) !== -1,
+	);
+	if (handlers.length) {
+		invalid(
+			meta,
+			`Command() was given the handler(s) ${handlers
+				.map((field) => `'${field}'`)
+				.join(", ")}; in the class form handlers are methods of the class`,
+		);
+	}
+
+	const unknownFields = fields.filter(
+		(field) => META_FIELDS.indexOf(field) === -1,
+	);
+	if (unknownFields.length) {
+		invalid(
+			meta,
+			`unknown field(s) ${unknownFields
+				.map((field) => `'${field}'`)
+				.join(", ")}; Command() accepts ${META_FIELDS.join(", ")}`,
+		);
+	}
+
+	validateDefinition({ ...meta, run: (): void => undefined });
+};
+
 /**
  * A definition that carries the name it declares in its own type. `Omit` rather
  * than an intersection: intersecting the declared name with the wider `name` of
@@ -629,10 +672,7 @@ export type CommandMeta<
  * every `Command()` class — a subclass's declaration emit refers to it — not
  * because anything should extend it directly.
  */
-export abstract class CommandBase<
-	TSchema extends CommandOptionsSchema = {},
-	TResult = void,
-> {
+export abstract class CommandBase<TSchema extends CommandOptionsSchema = {}> {
 	/**
 	 * The instance is built once per invocation, as that invocation's `setup`,
 	 * so the context captured here is the one its own run was handed.
@@ -647,9 +687,10 @@ export abstract class CommandBase<
 		return this.context.args;
 	}
 
-	abstract run(): Promise<TResult> | TResult;
+	abstract run(): unknown;
 	canExecute?(): Promise<boolean> | boolean;
-	postRun?(result: Awaited<TResult>): Promise<void> | void;
+	/** Typed off the subclass's own `run` through the polymorphic `this`. */
+	postRun?(result: Awaited<ReturnType<this["run"]>>): Promise<void> | void;
 	shortcuts?(): KeyShortcut[];
 }
 
@@ -661,24 +702,16 @@ export abstract class CommandBase<
 export type CommandClass<
 	TName extends CommandName = CommandName,
 	TSchema extends CommandOptionsSchema = {},
-	TResult = void,
-> = (abstract new () => CommandBase<TSchema, TResult>) & {
-	readonly definition: NamedCommand<
-		TSchema,
-		TResult,
-		CommandBase<TSchema, TResult>,
-		TName
-	>;
+> = (abstract new () => CommandBase<TSchema>) & {
+	readonly definition: NamedCommand<TSchema, any, CommandBase<TSchema>, TName>;
 	readonly [COMMAND_CLASS_MARKER]: true;
 };
 
 /** Either accepted form of a command, as a registration site takes it. */
 export type RegisterableCommand =
-	DefinedCommand<any, any, any> | CommandClass<any, any, any>;
+	DefinedCommand<any, any, any> | CommandClass<any, any>;
 
-export function isCommandClass(
-	value: any,
-): value is CommandClass<any, any, any> {
+export function isCommandClass(value: any): value is CommandClass<any, any> {
 	return (
 		typeof value === "function" && (<any>value)[COMMAND_CLASS_MARKER] === true
 	);
@@ -691,9 +724,19 @@ const buildClassDefinition = (ctor: any): DefinedCommand<any, any, any> => {
 		typeof prototype[method] === "function";
 
 	if (!implementsMethod("run")) {
+		// The base Command() returns is never the author's class, and its
+		// local name would only mislead.
+		const isFactoryBase = Object.prototype.hasOwnProperty.call(
+			ctor,
+			COMMAND_CLASS_META,
+		);
 		invalid(
 			meta,
-			`the class '${ctor.name || "<anonymous>"}' implements no 'run' method`,
+			isFactoryBase
+				? "the class returned by Command() implements no 'run' method; extend it with a class that does"
+				: ctor.name
+					? `the class '${ctor.name}' implements no 'run' method`
+					: "an anonymous class implements no 'run' method",
 		);
 	}
 
@@ -785,9 +828,10 @@ export type CommandReference = string | RegisterableCommand;
 export function Command<
 	const TName extends CommandName,
 	TSchema extends CommandOptionsSchema = {},
-	TResult = void,
->(meta: CommandMeta<TName, TSchema>): CommandClass<TName, TSchema, TResult> {
-	abstract class Base extends CommandBase<TSchema, TResult> {
+>(meta: CommandMeta<TName, TSchema>): CommandClass<TName, TSchema> {
+	validateMeta(meta);
+
+	abstract class Base extends CommandBase<TSchema> {
 		// A getter, because `this` in a static accessor is the constructor the
 		// property was read through: that is the only hook that resolves the
 		// subclass without the subclass having to name itself.

test/define-command.ts

@@ -2609,7 +2609,7 @@ describe("defineCommand", () => {
 			const testInjector = createTestInjector();
 			const order: string[] = [];
 
-			class Widget extends Command<"dctest-class-postrun", {}, number>({
+			class Widget extends Command({
 				name: "dctest-class-postrun",
 			}) {
 				public run(): number {
@@ -2726,6 +2726,66 @@ describe("defineCommand", () => {
 			);
 		});
 
+		describe("define-time validation", () => {
+			it("rejects an unusable name at the Command() call", () => {
+				assert.throws(
+					() => Command({ name: "" }),
+					/Invalid command definition for an unnamed command.*'name' must be/s,
+				);
+			});
+
+			it("rejects an unknown meta field at the Command() call, naming it", () => {
+				assert.throws(
+					() => Command(<any>{ name: "dctest-class-typo", typo: 1 }),
+					/Invalid command definition for 'dctest-class-typo'.*unknown field\(s\) 'typo'; Command\(\) accepts/s,
+				);
+			});
+
+			it("rejects a bad option spec at the Command() call", () => {
+				assert.throws(
+					() =>
+						Command({
+							name: "dctest-class-badoption",
+							options: { flag: <any>{ type: "flag" } },
+						}),
+					/Invalid command definition for 'dctest-class-badoption'.*option 'flag' has type 'flag'/s,
+				);
+			});
+
+			it("rejects a handler passed in the meta", () => {
+				assert.throws(
+					() =>
+						Command(<any>{
+							name: "dctest-class-metahandler",
+							canExecute(): boolean {
+								return true;
+							},
+						}),
+					/Invalid command definition for 'dctest-class-metahandler'.*handler\(s\) 'canExecute'; in the class form handlers are methods of the class/s,
+				);
+			});
+
+			it("leaves the missing run to the first definition read", () => {
+				const base: any = Command({ name: "dctest-class-lazyrun" });
+				const [Anonymous] = [class extends base {}];
+				class Named extends base {}
+
+				for (const ctor of [base, Anonymous, Named]) {
+					let message = "";
+					try {
+						ctor.definition;
+					} catch (err) {
+						message = err.message;
+					}
+
+					assert.match(message, /implements no 'run' method/);
+					assert.notInclude(message, "'Base'");
+				}
+
+				assert.throws(() => (<any>Named).definition, /the class 'Named'/);
+			});
+		});
+
 		it("reports a class Command() did not produce, through the deferred loader", () => {
 			const testInjector = createTestInjector();
 

test/type-fixtures/define-command-types.ts

@@ -275,10 +275,10 @@ class TypefixturePlatformClean extends Command({
 	}
 }
 
-class TypefixtureResult extends Command<"typefixture|class-result", {}, number>(
-	{ name: "typefixture|class-result" },
-) {
-	run(): number {
+// The result type is inferred from run; postRun receives it without the
+// class restating it.
+class TypefixtureResult extends Command({ name: "typefixture|class-result" }) {
+	run() {
 		return 1;
 	}
 
@@ -287,6 +287,44 @@ class TypefixtureResult extends Command<"typefixture|class-result", {}, number>(
 	}
 }
 
+class TypefixtureResultMismatch extends Command({
+	name: "typefixture|class-result-mismatch",
+}) {
+	run() {
+		return 1;
+	}
+
+	// @ts-expect-error - run returns a number, so postRun cannot take a string
+	postRun(result: string): void {
+		return undefined;
+	}
+}
+
+class TypefixtureAsyncResult extends Command({
+	name: "typefixture|class-async-result",
+}) {
+	async run() {
+		return { created: true, path: "/tmp/app" };
+	}
+
+	postRun(result: { created: boolean; path: string }): void {
+		return undefined;
+	}
+}
+
+class TypefixtureAsyncResultMismatch extends Command({
+	name: "typefixture|class-async-result-mismatch",
+}) {
+	async run() {
+		return { created: true };
+	}
+
+	// @ts-expect-error - postRun receives the awaited object, not a string flag
+	postRun(result: { created: string }): void {
+		return undefined;
+	}
+}
+
 // @ts-expect-error - run is abstract; a command class has to implement it
 class TypefixtureNoRun extends Command({ name: "typefixture|class-no-run" }) {}