fix(commands): make ctx.injector the invocation's own injector

The context handed to a handler named the injector the command was
registered against, while inject() inside the same handler resolved
against the per-invocation child that provides COMMAND_CONTEXT. The two
lookups the docs describe as one - inject() before the first await,
ctx.injector.get() after it - therefore hit different injectors, and
ctx.injector.get(COMMAND_CONTEXT) threw.

The context now carries the invocation's child injector. Per-command
providers move from a registration-time scope into that same child, so
a factory or class provider can inject the invocation, as the docs
already promised; the cost is one provider instance per invocation.

Author: edusperoni

defining-commands.md

@@ -344,7 +344,8 @@ The run context
   `const { args, arguments } = ctx` would not even parse.
 - `ctx.options` — the current value of each declared option, read at the moment
   the command executes.
-- `ctx.injector` — the injector this command was registered against; see
+- `ctx.injector` — this invocation's injector, a child of the one the command
+  was registered against; see
   [Injection, and the first `await`](#injection-and-the-first-await).
 - `ctx.fail(message)` — fails the command with `message` and a usage help
   suggestion.
@@ -414,10 +415,13 @@ async run(ctx) {
 `ctx.injector` is deliberately the injector itself rather than a bound
 `ctx.inject(...)`: it is a visibly different mechanism because it obeys
 different rules, and mistaking one for the other is exactly the bug this shape
-prevents. It is the injector the command was **registered against**, so it also
-resolves providers a child scope supplied — see
-[Registering a definition](#registering-a-definition). The same guidance, and
-the reasoning behind it, is in `dependency-injection.md`.
+prevents. It is the **invocation's own injector**: a child of the one the
+command was registered against, holding the context under `COMMAND_CONTEXT`
+and any per-command providers — see
+[Registering a definition](#registering-a-definition). `inject()` before the
+first `await` and `ctx.injector.get()` after it are therefore the same lookup
+against the same injector. The same guidance, and the reasoning behind it, is
+in `dependency-injection.md`.
 
 Where a handler gets its services
 ---------------------------------
@@ -597,8 +601,10 @@ schema does not declare is a compile error. `this.context` also carries
 **Per-command providers see the invocation.** The context is provided to the
 invocation's own child injector under the `COMMAND_CONTEXT` token, which is how
 the base class reads it. A provider registered for one command — through the
-`providers` argument of `registerCommand` or `registerLazyCommand` — can inject
-it too, and resolves nothing outside a running invocation.
+`providers` argument of `registerCommand` or `registerLazyCommand` — lives in
+that same child, so a factory or class among them can inject the context too.
+The cost is that such a provider is built once per invocation, never shared
+across invocations, and resolves nothing outside a running one.
 
 **One field per dependency.** Each service the class uses is its own field,
 read as `this.$x`:
@@ -678,8 +684,8 @@ the registry. It claims every name the definition declares, through the
 `DeferredCommandResult` — see _The owner is ambient_ below. The command instance
 is built by a factory on first resolution and cached.
 
-Pass providers as the second argument to scope the command to a child injector
-of the one it registers against — how a definition is parameterized per
+Pass providers as the second argument to add them to each invocation's child
+injector, the one `ctx.injector` names — how a definition is parameterized per
 registration:
 
 ```ts

lib/common/services/command-definition-adapter.ts

@@ -197,6 +197,7 @@ export function createCommandFromDefinition<
 >(
 	definition: CommandDefinition<TSchema, TResult, TSetup>,
 	targetInjector: Injector = <Injector>(<any>getRootInjector()),
+	providers: Provider[] = [],
 ): ICommand {
 	const schema = definition.options || <TSchema>{};
 	const optionNames = Object.keys(schema);
@@ -269,7 +270,10 @@ export function createCommandFromDefinition<
 			args,
 			params: mapArguments(args),
 			options,
-			injector: targetInjector,
+			// The invocation's child injector provides this very object under
+			// COMMAND_CONTEXT, so it can only be created - and assigned here -
+			// once the context exists.
+			injector: <Injector>undefined,
 			fail,
 		};
 	};
@@ -375,12 +379,19 @@ export function createCommandFromDefinition<
 	// opens one only when the current invocation has already run.
 	let currentInvocation: Invocation = null;
 
-	const beginInvocation = (context: CommandContext<TSchema>): Invocation => {
+	const beginInvocation = (args: string[]): Invocation => {
+		const context = buildContext(args);
+		// Per-command providers live here rather than in a registration-time
+		// scope so that a factory or class among them can inject the
+		// invocation; the price is one instance per invocation.
+		const injector = targetInjector.createChild([
+			{ provide: COMMAND_CONTEXT, useValue: context },
+			...providers,
+		]);
+		context.injector = injector;
 		const invocation: Invocation = {
 			context,
-			injector: targetInjector.createChild([
-				{ provide: COMMAND_CONTEXT, useValue: context },
-			]),
+			injector,
 			setup: undefined,
 			hasRun: false,
 		};
@@ -447,8 +458,7 @@ export function createCommandFromDefinition<
 			? {}
 			: {
 					postCommandAction: async (args: string[]): Promise<void> => {
-						const invocation =
-							currentInvocation || beginInvocation(buildContext(args));
+						const invocation = currentInvocation || beginInvocation(args);
 						const context = invocation.context;
 						const setupResult = await invocation.setup;
 						await runInInjectionContext(invocation.injector, () =>
@@ -462,13 +472,13 @@ export function createCommandFromDefinition<
 					},
 				}),
 		canExecute: async (args: string[]): Promise<boolean> => {
-			const context = buildContext(args);
 			// Setup first: it stands in for the constructor work legacy commands
 			// did at resolution time, which ran before anything looked at the
 			// arguments - so an argument validator can rely on it, and a command
 			// run in the wrong place still reports that before complaining about
 			// arity.
-			const invocation = beginInvocation(context);
+			const invocation = beginInvocation(args);
+			const context = invocation.context;
 			const setupResult = await invocation.setup;
 
 			await enforceArguments(context);
@@ -488,7 +498,7 @@ export function createCommandFromDefinition<
 			const invocation =
 				currentInvocation && !currentInvocation.hasRun
 					? currentInvocation
-					: beginInvocation(buildContext(args));
+					: beginInvocation(args);
 			const context = invocation.context;
 			invocation.hasRun = true;
 
@@ -518,14 +528,15 @@ export function registerDefinitionAs<
 	name: string,
 	definition: DefinedCommand<TSchema, TResult, TSetup>,
 	targetInjector: Injector = <Injector>(<any>getRootInjector()),
+	providers: Provider[] = [],
 ): void {
 	// The registry facet rather than the injector itself, so a child injector
 	// that provides its own CommandRegistry receives the registration.
 	const registry = targetInjector.get(CommandRegistry);
 	// A prototype-less zero-parameter function registers as a useFactory
 	// provider, so the command is built on first resolution and cached.
 	registry.registerCommand(name, () =>
-		createCommandFromDefinition(definition, targetInjector),
+		createCommandFromDefinition(definition, targetInjector, providers),
 	);
 }
 
@@ -574,8 +585,10 @@ export async function canExecuteCommand(
  * behalf.
  *
  * Registration targets the injector of the current injection context, and
- * `providers` scope the command to a child of it. To register against some
- * other injector, run the call in its context:
+ * `providers` are added to each invocation's own child of it, next to
+ * COMMAND_CONTEXT, so they can inject the invocation and are built once per
+ * invocation. To register against some other injector, run the call in its
+ * context:
  * `runInInjectionContext(injector, () => registerCommand(definition))`.
  *
  * Every registration has an owner and claims its names, the way
@@ -589,7 +602,7 @@ export function registerCommand<
 	TSetup = any,
 >(
 	definition:
-		| CommandClass<CommandName, TSchema, TResult>
+		| CommandClass<CommandName, TSchema>
 		| DefinedCommand<TSchema, TResult, TSetup>
 		| CommandDefinition<TSchema, TResult, TSetup>,
 	providers: Provider[] = [],
@@ -598,14 +611,13 @@ export function registerCommand<
 		toCommandDefinition(definition) ||
 		defineCommand(<CommandDefinition<TSchema, TResult, TSetup>>definition);
 	const target = contextInjector();
-	const scope = providers.length ? target.createChild(providers) : target;
 	const owner = target.get(COMMAND_OWNER, { optional: true }) || CLI_OWNER;
 	const registry = target.get(CommandRegistry);
 
 	for (const name of namesOf(defined)) {
 		const result = registry.registerDeferredCommand(name, {
 			owner,
-			load: () => registerDefinitionAs(name, defined, scope),
+			load: () => registerDefinitionAs(name, defined, target, providers),
 		});
 
 		if (!result.registered) {
@@ -639,8 +651,9 @@ type MissingTypeArgument =
  *       () => require("./commands/run").iosRunCommand,
  *     );
  *
- * `providers` scope the command to a child injector, built when the command is
- * constructed rather than when its name is claimed.
+ * `providers` are added to each invocation's own child injector, as for
+ * `registerCommand`, so nothing about them is resolved when the name is
+ * claimed.
  *
  * Registration targets the injector of the current injection context, if there
  * is one, and takes its owner from that injector's COMMAND_OWNER — which is
@@ -712,11 +725,7 @@ export function registerLazyCommand<
 				);
 			}
 
-			registerDefinitionAs(
-				commandName,
-				definition,
-				providers.length ? target.createChild(providers) : target,
-			);
+			registerDefinitionAs(commandName, definition, target, providers);
 		},
 	});
 }

test/define-command.ts

@@ -405,7 +405,7 @@ describe("defineCommand", () => {
 			);
 
 			await testInjector.resolveCommand("dctestambient").execute([]);
-			assert.strictEqual(seenInjector, scope);
+			assert.strictEqual(seenInjector.parent, scope);
 
 			assert.deepStrictEqual(
 				runInInjectionContext(testInjector, () =>
@@ -2284,7 +2284,7 @@ describe("defineCommand", () => {
 			);
 		});
 
-		it("scopes the command to a child injector built on first resolution", async () => {
+		it("adds the providers to each invocation's injector, not at resolution", async () => {
 			const testInjector = createTestInjector();
 			const GREETING = new InjectionToken<string>("dctestLazyGreeting");
 			let seen: string;
@@ -2314,11 +2314,12 @@ describe("defineCommand", () => {
 			assert.strictEqual(children, 0);
 
 			const command = testInjector.resolveCommand("dctestlazyscoped");
-			assert.strictEqual(children, 1);
+			assert.strictEqual(children, 0);
 
 			await command.execute([]);
+			assert.strictEqual(children, 1);
 			assert.strictEqual(seen, "hello");
-			// The provider lives in the command's own scope, not the injector the
+			// The provider lives in the invocation's own injector, not the one the
 			// registration was made against.
 			assert.isNotOk((<any>testInjector).get(GREETING, { optional: true }));
 		});
@@ -2362,7 +2363,7 @@ describe("defineCommand", () => {
 			assert.deepStrictEqual(result, { registered: true });
 
 			await testInjector.resolveCommand("dctestlazyambient").execute([]);
-			assert.strictEqual(seenInjector, scope);
+			assert.strictEqual(seenInjector.parent, scope);
 
 			assert.deepStrictEqual(
 				runInInjectionContext(testInjector, () =>
@@ -2430,23 +2431,50 @@ describe("defineCommand", () => {
 	});
 
 	describe("ctx.injector", () => {
-		it("is the injector the command was registered against", async () => {
+		it("is the invocation's injector, a child of the registration injector", async () => {
 			const testInjector = createTestInjector();
+			testInjector.register("dcTestRegistered", { value: "parent" });
 			let seen: any;
+			let seenContext: any;
+			let injectedContext: any;
 
 			const command = createCommandFromDefinition(
 				defineCommand({
 					name: "dctest-injector",
 					run: (ctx) => {
+						injectedContext = inject(COMMAND_CONTEXT);
 						seen = ctx.injector;
+						seenContext = ctx.injector.get(COMMAND_CONTEXT);
 					},
 				}),
 				testInjector,
 			);
 
 			await command.execute([]);
 
-			assert.strictEqual(seen, testInjector);
+			assert.notStrictEqual(seen, testInjector);
+			assert.strictEqual(seenContext, injectedContext);
+			assert.strictEqual(seen.get("dcTestRegistered").value, "parent");
+		});
+
+		it("is a different injector for each invocation", async () => {
+			const testInjector = createTestInjector();
+			const seen: any[] = [];
+
+			const command = createCommandFromDefinition(
+				defineCommand({
+					name: "dctest-injector-per-invocation",
+					run: (ctx) => {
+						seen.push(ctx.injector);
+					},
+				}),
+				testInjector,
+			);
+
+			await command.execute([]);
+			await command.execute([]);
+
+			assert.notStrictEqual(seen[0], seen[1]);
 		});
 
 		it("resolves after the first await, where inject() no longer can", async () => {
@@ -2858,5 +2886,49 @@ describe("defineCommand", () => {
 
 			assert.deepEqual(ran, ["android:true:android", "ios:true:ios"]);
 		});
+
+		it("builds a factory provider per invocation, with the invocation in reach", async () => {
+			const LABEL = new InjectionToken<{ label: string }>("dcTestLabel");
+			const testInjector = createTestInjector();
+			const built: string[] = [];
+			const seen: any[] = [];
+
+			runInInjectionContext(testInjector, () =>
+				registerCommand(
+					defineCommand({
+						name: "dctest-provider-sees-invocation",
+						arguments: "any",
+						run: (ctx) => {
+							const first = ctx.injector.get(LABEL);
+							seen.push(first, ctx.injector.get(LABEL));
+						},
+					}),
+					[
+						{
+							provide: LABEL,
+							useFactory: () => {
+								const label = inject(COMMAND_CONTEXT).args.join("+");
+								built.push(label);
+								return { label };
+							},
+						},
+					],
+				),
+			);
+
+			const command = testInjector.resolveCommand(
+				"dctest-provider-sees-invocation",
+			);
+			await command.execute(["a", "b"]);
+			await command.execute(["c"]);
+
+			assert.deepEqual(built, ["a+b", "c"]);
+			assert.strictEqual(seen[0], seen[1]);
+			assert.notStrictEqual(seen[0], seen[2]);
+			assert.deepEqual(
+				seen.map((entry) => entry.label),
+				["a+b", "a+b", "c", "c"],
+			);
+		});
 	});
 });