fix: address review findings on the DI foundation

- doctor-service: printWarnings accepts an optional trackResult matching
  its contracts; runSetupScript returns the setup script result instead
  of resolving undefined; canExecuteLocalBuild no longer dereferences
  its optional argument
- deprecation tracer: a report dropped for lack of a logger is no
  longer latched as delivered - it reports once a logger exists
- yok: global.$injector is an accessor pair, so a direct third-party
  assignment stays synchronized with the binding getInjector() reads
- docs: note the useValue + shared:false quirk; show where Injector is
  imported from in the late-lookup example

Author: edusperoni

dependency-injection.md

@@ -123,7 +123,7 @@ Provider kinds:
 |---|---|---|
 | Class | `provide(Token, Impl)` / `{ provide, useClass }` | constructed with `new Impl()` inside an injection context, so `inject()` works in its fields |
 | Lazy class | `provideLazy(Token, () => Impl)` / `{ provide, useLazyClass }` | loader runs on first `get()` only — keeps startup lazy |
-| Value | `{ provide, useValue }` | registered instance; re-registering replaces the cached instance |
+| Value | `{ provide, useValue }` | registered instance; re-registering replaces the cached instance. With `shared: false` there is no resolver and `get()` throws — a preserved legacy quirk |
 | Factory | `{ provide, useFactory }` | called inside an injection context |
 
 `shared: false` makes a provider transient: every resolution constructs a fresh

extending-cli.md

@@ -92,7 +92,7 @@ module.exports = function (hookArgs) {
 };
 ```
 
-* `inject()` is valid in the synchronous part of the hook body — not after an `await`. Resolve what you need up front; for late lookups, grab the container first: `const injector = inject(Injector)`, then `injector.get(...)` later.
+* `inject()` is valid in the synchronous part of the hook body — not after an `await`. Resolve what you need up front; for late lookups, grab the container first: `const injector = inject(Injector)` (`Injector` is exported from `nativescript/contracts` too), then `injector.get(...)` later.
 * `hookArgs` contains the parameters of the CLI function being hooked; its shape depends on the hook point. Declare it only when you need it — a hook may also take no parameters at all. A future typed hook API (`defineHook` with an explicit context object) will replace this parameter; it is the one remaining piece of the legacy convention.
 * Tokens resolve by class first and by their canonical name on a miss, so this works even if your dependency tree carries its own copy of `nativescript` — a duplicated token class still resolves to the running CLI's service.
 * Only a first tranche of services has typed tokens so far ([dependency-injection.md](dependency-injection.md#available-contracts) lists them); a service without a token is reachable by its registry name — `inject("logger")` — as a migration bridge.

lib/common/deprecation.ts

@@ -42,12 +42,14 @@ export function reportDeprecation(report: IDeprecationReport): void {
 	if (reported.has(key)) {
 		return;
 	}
-	reported.add(key);
 
 	const logger = report.logger || tryResolveGlobalLogger();
 	if (!logger) {
+		// Deliberately not latched: a report dropped for lack of a logger must
+		// still be deliverable once one becomes resolvable.
 		return;
 	}
+	reported.add(key);
 
 	if (stage === "warn") {
 		logger.warn(message);

lib/common/yok.ts

@@ -604,10 +604,18 @@ export class Yok extends Injector implements IInjector {
 	}
 }
 
-if (!(<any>global).$injector) {
-	(<any>global).$injector = new Yok();
-	injector = (<any>global).$injector;
-}
+// The global is the published legacy surface. It is an accessor pair so a
+// direct `global.$injector = x` assignment — allowed for third parties —
+// stays synchronized with the module binding that getInjector() and internal
+// code read; a plain data property would silently fork the two.
+injector = (<any>global).$injector || new Yok();
+Object.defineProperty(global, "$injector", {
+	get: () => injector,
+	set: (value: IInjector) => {
+		injector = value;
+	},
+	configurable: true,
+});
 
 /**
  * Accessor for the process-wide facade, for code that cannot receive the
@@ -625,6 +633,7 @@ export function getInjector(): IInjector {
  * the container via inject(Injector) instead of a process-wide global.
  */
 export function setGlobalInjector(inj: IInjector): IInjector {
-	(<any>global).$injector = injector = inj;
+	injector = inj;
+	(<any>global).$injector = inj;
 	return inj;
 }

lib/services/doctor-service.ts

@@ -79,7 +79,7 @@ export class DoctorServiceImpl
 	) {}
 
 	public async printWarnings(configOptions?: {
-		trackResult: boolean;
+		trackResult?: boolean;
 		projectDir?: string;
 		runtimeVersion?: string;
 		options?: IOptions;
@@ -177,15 +177,16 @@ export class DoctorServiceImpl
 			"Running the setup script to try and automatically configure your environment.",
 		);
 
+		let result: ISpawnResult;
 		if (this.$hostInfo.isDarwin) {
-			await this.runSetupScriptCore(
+			result = await this.runSetupScriptCore(
 				DoctorServiceImpl.DarwinSetupScriptLocation,
 				[],
 			);
 		}
 
 		if (this.$hostInfo.isWindows) {
-			await this.runSetupScriptCore(
+			result = await this.runSetupScriptCore(
 				DoctorServiceImpl.WindowsSetupScriptExecutable,
 				DoctorServiceImpl.WindowsSetupScriptArguments,
 			);
@@ -195,6 +196,8 @@ export class DoctorServiceImpl
 			action: TrackActionNames.RunSetupScript,
 			additionalData: "Finished",
 		});
+
+		return result;
 	}
 
 	public async canExecuteLocalBuild(configuration?: {
@@ -203,6 +206,7 @@ export class DoctorServiceImpl
 		runtimeVersion?: string;
 		forceCheck?: boolean;
 	}): Promise<boolean> {
+		configuration = configuration || {};
 		await this.$analyticsService.trackEventActionInGoogleAnalytics({
 			action: TrackActionNames.CheckLocalBuildSetup,
 			additionalData: "Starting",

test/compat/injector-facade-surface.ts

@@ -1,5 +1,5 @@
 import { assert } from "chai";
-import { Yok } from "../../lib/common/yok";
+import { Yok, getInjector } from "../../lib/common/yok";
 import { Injector, inject, runInInjectionContext } from "../../lib/common/di";
 import {
 	CommandRegistry,
@@ -62,6 +62,19 @@ describe("injector facade surface", () => {
 		assert.strictEqual(sub.resolve("injector"), sub);
 	});
 
+	it("keeps getInjector() synchronized with a direct global.$injector assignment", () => {
+		const previous = getInjector();
+		const fresh = new Yok();
+
+		(<any>global).$injector = fresh;
+		try {
+			assert.strictEqual(getInjector(), fresh);
+		} finally {
+			(<any>global).$injector = previous;
+		}
+		assert.strictEqual(getInjector(), previous);
+	});
+
 	it("assigns the process-wide global.$injector", () => {
 		assert.isOk((<any>global).$injector);
 		for (const member of FACADE_METHODS) {

test/deprecation.ts

@@ -84,4 +84,18 @@ describe("deprecation tracer", () => {
 			setGlobalInjector(previousInjector);
 		}
 	});
+
+	it("still delivers a report that was previously dropped for lack of a logger", () => {
+		const previousInjector = getInjector();
+		setGlobalInjector(new Yok());
+		try {
+			reportDeprecation({ api: "test.redeliver" });
+		} finally {
+			setGlobalInjector(previousInjector);
+		}
+
+		reportDeprecation({ api: "test.redeliver", logger });
+
+		assert.include(logger.traceOutput, "test.redeliver");
+	});
 });