From f29862a9bcff0338d9c567b54f8a9881776d6212 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 21:59:49 -0300 Subject: [PATCH 01/18] feat(commands): add defineCommand with typed options via an ICommand adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commands can now be declared as plain objects: a name, an option schema built from booleanOption/stringOption/numberOption/arrayOption, and a run function whose context carries the positional args plus the declared options, typed by inference from the schema. lib/common/define-command holds the types and the pure factories only, so it stays side-effect-free and can be re-exported from nativescript/contracts. The runtime bridge lives in lib/common/services/command-definition-adapter, which compiles a definition into the ICommand the legacy registry expects and runs it inside an injection context. canExecute is emitted only when the definition supplies one or opts into arguments: "any"; CommandsService skips all parameter validation as soon as canExecute exists, so omitting it is what lets the framework reject stray positional arguments for arguments: "none". Fully additive — existing ICommand classes are untouched. --- defining-commands.md | 251 ++++++++++ lib/common/define-command.ts | 104 ++++ .../services/command-definition-adapter.ts | 120 +++++ lib/contracts/index.ts | 18 + test/define-command.ts | 461 ++++++++++++++++++ 5 files changed, 954 insertions(+) create mode 100644 defining-commands.md create mode 100644 lib/common/define-command.ts create mode 100644 lib/common/services/command-definition-adapter.ts create mode 100644 test/define-command.ts diff --git a/defining-commands.md b/defining-commands.md new file mode 100644 index 0000000000..fa0143123b --- /dev/null +++ b/defining-commands.md @@ -0,0 +1,251 @@ +Defining Commands +================= + +`defineCommand` is the declarative way to add a command to the NativeScript +CLI. A definition is a plain object: a name, an option schema, and a `run` +function. The CLI compiles it into the command shape its registry expects, so a +definition gets the same option parsing, hooks, analytics and help wiring as a +hand-written command class — without a class, a constructor, or an +`allowedParameters` array. + +This is purely additive. The legacy `ICommand` classes registered through +`$injector.registerCommand` keep working exactly as before, and the two styles +coexist in the same registry. + +At a glance +----------- + +```ts +import { + defineCommand, + booleanOption, + stringOption, +} from "nativescript/contracts"; + +export default defineCommand({ + name: "widget|add", + description: "Adds a widget to the project", + options: { + verbose: booleanOption({ default: false }), + output: stringOption({ alias: "o" }), + }, + arguments: "any", + async run(ctx) { + // ctx.args -> string[] of positional arguments + // ctx.options -> { verbose: boolean; output: string } + if (ctx.options.verbose) { + console.log(`adding ${ctx.args.join(", ")} to ${ctx.options.output}`); + } + }, +}); +``` + +`defineCommand` returns the definition, tagged with a marker symbol so that any +copy of the CLI can recognise it (`isCommandDefinition(value)` is the exported +predicate). It does not register anything by itself — see +[Registering a definition](#registering-a-definition). + +Names and the command hierarchy +------------------------------- + +`name` is either a single string or an array of strings, in which case every +entry becomes an alias for the same command. + +The CLI's command registry is flat; the hierarchy the user types on the command +line is encoded in the name with a `|` separator. `"widget|add"` is the command +invoked as `ns widget add`, and `"widget|template|list"` is `ns widget template +list`. Registering a hierarchical name automatically synthesises the parent +dispatcher (`widget`), which routes to the right subcommand or prints help. + +A leading `*` on the last segment marks a **default subcommand**: `"widget|*add"` +runs both for `ns widget add` and for a bare `ns widget`. This is the convention +the CLI's own commands use (`run|*all`, `debug|*all`); the encoding is +user-visible because it feeds shell autocompletion and generated help. + +Options +------- + +`options` is a schema keyed by the long option name — `verbose` is passed as +`--verbose`. Declare each entry with one of the four helpers, which fix the +value type: + +| Helper | Value type on `ctx.options` | +| --------------- | --------------------------- | +| `booleanOption` | `boolean` | +| `stringOption` | `string` | +| `numberOption` | `number` | +| `arrayOption` | `string[]` | + +Each helper takes an optional spec: + +```ts +options: { + // --release, absent means false + release: booleanOption({ default: false }), + // --output , also accepted as -o + output: stringOption({ alias: "o", description: "Output directory" }), + // --retries + retries: numberOption({ default: 3 }), + // --file a.ts --file b.ts + file: arrayOption(), + // kept out of analytics and logs + token: stringOption({ hasSensitiveValue: true }), +} +``` + +- `default` — value used when the flag is absent. +- `alias` — single-dash shorthand, or an array of them. +- `hasSensitiveValue` — defaults to `false`; set it for anything that must not + be recorded. There is no reason not to be explicit about credentials, paths + containing user directories, and tokens. +- `description` — shown in generated help. + +The schema types `ctx.options` and nothing else: `ctx.options` carries exactly +the declared keys, with the types the helpers imply. Values that the CLI parses +globally (`--path`, `--log`, …) are not exposed there; resolve the `options` +service if you need them. + +### How validation behaves + +Option validation is the CLI's existing behaviour, not something the definition +opts into. Before a command runs, the parser is re-primed with that command's +declared options and the command line is re-parsed: + +- Declared options are accepted and appear on `ctx.options`. +- An option the CLI does not know — neither global nor declared by this command + — is a **hard error**: the command does not run, and the CLI prints + `The option '' is not supported.` followed by a help suggestion. + +So adding an option is exactly a matter of adding a schema entry; forgetting to +declare one that users pass is a failure, not a silent `undefined`. + +Positional arguments +-------------------- + +`arguments` declares whether the command takes positional arguments at all: + +- `"none"` (the default) — the command accepts no positional arguments. Passing + any is rejected with `This command doesn't accept parameters.` +- `"any"` — positional arguments are accepted and handed to `run` as + `ctx.args`. + +Anything finer than that belongs in `canExecute`. + +### `canExecute` owns validation + +```ts +defineCommand({ + name: "widget|add", + arguments: "any", + async canExecute(ctx) { + return ctx.args.length === 1; + }, + async run(ctx) { + /* ... */ + }, +}); +``` + +`canExecute` receives the same context as `run` and returns a boolean (or a +promise of one). Returning `false` aborts the command and prints a help +suggestion; throwing surfaces your own error message, which is usually the +friendlier choice. + +There is one rule to internalise: **the moment a command supplies +`canExecute`, it owns argument validation completely.** The framework returns +that verdict and skips every built-in parameter check, including the +"no parameters" rule implied by `arguments: "none"`. A definition with a +`canExecute` that only inspects options will therefore accept stray positional +arguments unless it checks `ctx.args` itself. If you do not need custom +validation, omit `canExecute` and let `arguments` do the work. + +The run context +--------------- + +`run(ctx)` receives: + +- `ctx.args` — `string[]`, the positional arguments left after the command name + (including any subcommand segments) has been consumed. +- `ctx.options` — the current value of each declared option, read at the moment + the command executes. + +`run` may be synchronous or `async`; the CLI awaits the result and treats a +rejection as a command failure. + +`run` starts inside a dependency-injection context, so `inject()` works +directly: + +```ts +import { defineCommand, inject } from "nativescript/contracts"; +import { DoctorService } from "nativescript/contracts"; + +export default defineCommand({ + name: "widget|check", + async run() { + const doctorService = inject(DoctorService); + await doctorService.printWarnings(); + }, +}); +``` + +The injection context is synchronous: `inject()` is valid up to the first +`await` in `run`, and not after it. Capture what you need at the top of `run`, +or inject the `Injector` itself and use `injector.get()` for late lookups. See +`dependency-injection.md`. + +Other flags +----------- + +- `disableAnalytics: true` — skips analytics tracking for this command. +- `enableHooks: false` — skips the before/after hooks that normally run around + the command. Hooks are enabled by default. + +Both are simply passed through to the command the CLI executes; omitting them +leaves the CLI's defaults in place. + +Registering a definition +------------------------ + +Inside the CLI, a definition is registered with `registerCommandDefinition`: + +```ts +import { registerCommandDefinition } from "../common/services/command-definition-adapter"; +import addWidgetCommand from "./add-widget"; + +registerCommandDefinition(addWidgetCommand); +``` + +It registers the definition under every name it declares, on the CLI's injector +by default; pass a second argument to target a different injector (tests do +this). The command instance is created on first resolution and cached. + +`registerCommandDefinition` lives in +`lib/common/services/command-definition-adapter` rather than in +`nativescript/contracts`, because it reaches into the CLI runtime — the +side-effect-free contracts entry point deliberately does not pull it in. +`defineCommand`, the option helpers and all the types are exported from both +`nativescript/contracts` and `lib/common/define-command`. + +Declaring commands from an extension manifest, so that an extension does not +have to call a registration function at load time, is being added separately. +Until then, extensions register definitions the same way the CLI does. + +Relationship to `ICommand` +-------------------------- + +A definition is compiled into an ordinary `ICommand`, so nothing downstream — +the registry, the router, hooks, help, analytics — knows the difference. The +mapping is: + +| Definition | `ICommand` | +| --------------------------------- | ------------------------------------------ | +| `options` | `dashedOptions` | +| `run` | `execute`, wrapped in an injection context | +| `arguments`, `canExecute` | `canExecute` (see the rule above) | +| — | `allowedParameters`, always `[]` | +| `disableAnalytics`, `enableHooks` | passed through unchanged | + +Existing command classes need no migration. Reach for a definition when a +command is mostly "parse these flags and do this"; a class still makes sense +when a command needs constructor-injected collaborators shared across several +methods, custom `ICommandParameter` validators, or a `postCommandAction`. diff --git a/lib/common/define-command.ts b/lib/common/define-command.ts new file mode 100644 index 0000000000..b197709ca9 --- /dev/null +++ b/lib/common/define-command.ts @@ -0,0 +1,104 @@ +/** + * The declarative command API. Types and pure factories only — this module is + * re-exported from `nativescript/contracts` and must stay side-effect-free, so + * it may not import lib/common/yok (whose import creates global.$injector). + * The runtime bridge onto the legacy registry lives in + * lib/common/services/command-definition-adapter. + */ + +/** + * Symbol.for so that a definition produced by one copy of the CLI is still + * recognised by another — extensions bundle their own node_modules. + */ +export const COMMAND_DEFINITION_MARKER = Symbol.for( + "nativescript:cli:commandDefinition", +); + +export type CommandOptionType = "boolean" | "string" | "number" | "array"; + +export interface ICommandOptionSpec { + type: CommandOptionType; + /** Value used when the flag is absent from the command line. */ + default?: TValue; + /** Single-dash shorthand, e.g. `-o` for `--output`. */ + alias?: string | string[]; + /** Keeps the value out of analytics and logs. Defaults to false. */ + hasSensitiveValue?: boolean; + description?: string; +} + +/** The parts of an option spec a caller supplies; `type` comes from the helper. */ +export type CommandOptionSpecInit = Omit< + ICommandOptionSpec, + "type" +>; + +export interface ICommandOptionsSchema { + [optionName: string]: ICommandOptionSpec; +} + +export type CommandOptionValues = { + [K in keyof TSchema]: TSchema[K] extends ICommandOptionSpec + ? TValue + : any; +}; + +export interface ICommandContext< + TSchema extends ICommandOptionsSchema = ICommandOptionsSchema, +> { + /** Positional arguments, after the command name has been consumed. */ + args: string[]; + /** Current value of every option declared in the schema, and nothing else. */ + options: CommandOptionValues; +} + +export interface ICommandDefinition< + TSchema extends ICommandOptionsSchema = ICommandOptionsSchema, +> { + /** `"widget|add"`; `|` separates hierarchy levels. Several names alias one command. */ + name: string | string[]; + description?: string; + options?: TSchema; + /** + * `"none"` (the default) rejects positional arguments; `"any"` accepts them. + * Anything finer belongs in `canExecute`. + */ + arguments?: "none" | "any"; + canExecute?(context: ICommandContext): Promise | boolean; + disableAnalytics?: boolean; + enableHooks?: boolean; + run(context: ICommandContext): Promise | void; +} + +const optionSpec = ( + type: CommandOptionType, + init: CommandOptionSpecInit, +): ICommandOptionSpec => ({ ...init, type }); + +export const booleanOption = ( + init: CommandOptionSpecInit = {}, +): ICommandOptionSpec => optionSpec("boolean", init); + +export const stringOption = ( + init: CommandOptionSpecInit = {}, +): ICommandOptionSpec => optionSpec("string", init); + +export const numberOption = ( + init: CommandOptionSpecInit = {}, +): ICommandOptionSpec => optionSpec("number", init); + +export const arrayOption = ( + init: CommandOptionSpecInit = {}, +): ICommandOptionSpec => optionSpec("array", init); + +export function defineCommand( + definition: ICommandDefinition, +): ICommandDefinition { + const marked: ICommandDefinition = { ...definition }; + (marked)[COMMAND_DEFINITION_MARKER] = true; + return marked; +} + +export function isCommandDefinition(value: any): value is ICommandDefinition { + return !!value && (value)[COMMAND_DEFINITION_MARKER] === true; +} diff --git a/lib/common/services/command-definition-adapter.ts b/lib/common/services/command-definition-adapter.ts new file mode 100644 index 0000000000..0956aa1db7 --- /dev/null +++ b/lib/common/services/command-definition-adapter.ts @@ -0,0 +1,120 @@ +import { OptionType } from "../enums"; +import { injector } from "../yok"; +import { runInInjectionContext } from "../di/inject"; +import { IDictionary, IDashedOption } from "../declarations"; +import { IInjector } from "../definitions/yok"; +import { ICommand } from "../definitions/commands"; +import { + CommandOptionType, + ICommandContext, + ICommandDefinition, + ICommandOptionsSchema, +} from "../define-command"; + +const OPTION_TYPES: IDictionary = { + boolean: OptionType.Boolean, + string: OptionType.String, + number: OptionType.Number, + array: OptionType.Array, +}; + +/** + * Wraps a declarative definition in the ICommand shape the legacy registry and + * CommandsService expect. + * + * Constraint worth knowing before changing the canExecute mapping: the moment + * an ICommand exposes `canExecute`, CommandsService returns its verdict and + * skips `allowedParameters` validation entirely. So `canExecute` is emitted + * only when the definition supplies one or opts into `arguments: "any"` — the + * adapter then owns argument validation wholesale. With `arguments: "none"` + * and no user `canExecute` the property is omitted, which lets the framework's + * own empty-`allowedParameters` check reject stray positional arguments. + */ +export function createCommandFromDefinition< + TSchema extends ICommandOptionsSchema, +>( + definition: ICommandDefinition, + targetInjector: IInjector = injector, +): ICommand { + const schema = definition.options || {}; + const optionNames = Object.keys(schema); + + const dashedOptions: IDictionary = {}; + for (const optionName of optionNames) { + const spec = schema[optionName]; + const dashedOption: IDashedOption = { + type: OPTION_TYPES[spec.type], + hasSensitiveValue: spec.hasSensitiveValue === true, + }; + + if (spec.default !== undefined) { + dashedOption.default = spec.default; + } + + if (spec.alias !== undefined) { + dashedOption.alias = spec.alias; + } + + if (spec.description !== undefined) { + dashedOption.describe = spec.description; + } + + dashedOptions[optionName] = dashedOption; + } + + // Resolved per call: the options service only holds parsed values once + // validateOptions has run for this command. + const buildContext = (args: string[]): ICommandContext => { + const optionsService: any = targetInjector.resolve("options"); + const options: any = {}; + for (const optionName of optionNames) { + options[optionName] = optionsService + ? optionsService[optionName] + : undefined; + } + + return { args, options }; + }; + + const command: ICommand = { + allowedParameters: [], + dashedOptions, + execute: async (args: string[]): Promise => { + // runInInjectionContext is synchronous, so inject() is available while + // run() executes up to its first await, and not after it. + await runInInjectionContext((targetInjector).di, () => + definition.run(buildContext(args)), + ); + }, + }; + + if (definition.canExecute || definition.arguments === "any") { + command.canExecute = async (args: string[]): Promise => + definition.canExecute + ? await definition.canExecute(buildContext(args)) + : true; + } + + if (definition.disableAnalytics !== undefined) { + command.disableAnalytics = definition.disableAnalytics; + } + + if (definition.enableHooks !== undefined) { + command.enableHooks = definition.enableHooks; + } + + return command; +} + +export function registerCommandDefinition< + TSchema extends ICommandOptionsSchema, +>( + definition: ICommandDefinition, + targetInjector: IInjector = injector, +): void { + // An arrow function resolver is invoked as a factory rather than new-ed, and + // its result is cached as the command instance. + targetInjector.registerCommand(definition.name, () => + createCommandFromDefinition(definition, targetInjector), + ); +} diff --git a/lib/contracts/index.ts b/lib/contracts/index.ts index c13a9b9996..efbdc504f6 100644 --- a/lib/contracts/index.ts +++ b/lib/contracts/index.ts @@ -25,3 +25,21 @@ export type { export { DoctorService } from "./doctor-service"; export { ProjectNameService } from "./project-name-service"; + +export { + defineCommand, + isCommandDefinition, + booleanOption, + stringOption, + numberOption, + arrayOption, +} from "../common/define-command"; +export type { + ICommandDefinition, + ICommandContext, + ICommandOptionSpec, + ICommandOptionsSchema, + CommandOptionSpecInit, + CommandOptionType, + CommandOptionValues, +} from "../common/define-command"; diff --git a/test/define-command.ts b/test/define-command.ts new file mode 100644 index 0000000000..011ff59f31 --- /dev/null +++ b/test/define-command.ts @@ -0,0 +1,461 @@ +import { assert } from "chai"; +import { Yok, injector as globalInjector } from "../lib/common/yok"; +import { IInjector } from "../lib/common/definitions/yok"; +import { inject } from "../lib/common/di"; +import { CommandsService } from "../lib/common/services/commands-service"; +import { LoggerStub, HooksServiceStub } from "./stubs"; +import { + arrayOption, + booleanOption, + defineCommand, + isCommandDefinition, + numberOption, + stringOption, +} from "../lib/common/define-command"; +import { + createCommandFromDefinition, + registerCommandDefinition, +} from "../lib/common/services/command-definition-adapter"; + +type IsExact = + (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 + ? true + : false; + +/** Fails to compile unless the schema inferred exactly the expected type. */ +const expectExactType = (): void => undefined; + +const createTestInjector = (options: any = {}): IInjector => { + const testInjector = new Yok(); + testInjector.register("options", options); + return testInjector; +}; + +describe("defineCommand", () => { + it("marks definitions so a duplicated CLI copy still recognises them", () => { + const definition = defineCommand({ + name: "dctest-marker", + run: () => undefined, + }); + + assert.isTrue(isCommandDefinition(definition)); + assert.isTrue( + (definition)[Symbol.for("nativescript:cli:commandDefinition")], + ); + assert.isFalse(isCommandDefinition({ name: "dctest-marker" })); + assert.isFalse(isCommandDefinition(null)); + }); + + describe("registration", () => { + it("round-trips through the legacy command registry", () => { + const definition = defineCommand({ + name: "dctestwidget|add", + description: "Adds a widget", + run: () => undefined, + }); + + // The parent dispatcher is synthesized onto the global facade by the + // legacy registry, so registration happens there too. + registerCommandDefinition(definition, globalInjector); + + const command = globalInjector.resolveCommand("dctestwidget|add"); + assert.isFunction(command.execute); + assert.deepEqual(command.allowedParameters, []); + + const parent = globalInjector.resolveCommand("dctestwidget"); + assert.isTrue(parent.isHierarchicalCommand); + + assert.include( + globalInjector.getRegisteredCommandsNames(false), + "dctestwidget|add", + ); + }); + + it("caches one command instance per registered name", () => { + const testInjector = createTestInjector(); + registerCommandDefinition( + defineCommand({ name: "dctestflat", run: () => undefined }), + testInjector, + ); + + assert.strictEqual( + testInjector.resolveCommand("dctestflat"), + testInjector.resolveCommand("dctestflat"), + ); + }); + + it("registers every alias of a multi-name definition", () => { + const testInjector = createTestInjector(); + registerCommandDefinition( + defineCommand({ + name: ["dctestalias", "dctestalias2"], + run: () => undefined, + }), + testInjector, + ); + + assert.isFunction(testInjector.resolveCommand("dctestalias").execute); + assert.isFunction(testInjector.resolveCommand("dctestalias2").execute); + }); + }); + + describe("execute", () => { + it("passes args and the declared options through, inside an injection context", async () => { + const testInjector = createTestInjector({ + verbose: true, + output: "dist", + undeclared: "ignored", + }); + testInjector.register("dcTestGreeter", { greet: () => "hello" }); + + let capturedArgs: string[]; + let capturedOptions: any; + let greeting: string; + + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestexec", + options: { + verbose: booleanOption(), + output: stringOption(), + }, + run(context) { + greeting = inject("dcTestGreeter").greet(); + capturedArgs = context.args; + capturedOptions = context.options; + }, + }), + testInjector, + ); + + await command.execute(["one", "two"]); + + assert.deepEqual(capturedArgs, ["one", "two"]); + assert.deepEqual(capturedOptions, { verbose: true, output: "dist" }); + assert.strictEqual(greeting, "hello"); + }); + + it("reads option values at execution time", async () => { + const optionsService: any = { verbose: false }; + const testInjector = createTestInjector(optionsService); + + let seen: boolean; + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestlate", + options: { verbose: booleanOption() }, + run: (context) => { + seen = context.options.verbose; + }, + }), + testInjector, + ); + + optionsService.verbose = true; + await command.execute([]); + + assert.isTrue(seen); + }); + + it("awaits an asynchronous run", async () => { + const testInjector = createTestInjector(); + let finished = false; + + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestasync", + run: async () => { + await new Promise((resolve) => setTimeout(resolve, 1)); + finished = true; + }, + }), + testInjector, + ); + + await command.execute([]); + + assert.isTrue(finished); + }); + + it("infers the option value types on the run context", async () => { + const testInjector = createTestInjector({ + verbose: true, + output: "dist", + retries: 3, + files: ["a.ts"], + }); + + let verbose: boolean; + let output: string; + let retries: number; + let files: string[]; + + const command = createCommandFromDefinition( + defineCommand({ + name: "dctesttypes", + options: { + verbose: booleanOption(), + output: stringOption(), + retries: numberOption(), + files: arrayOption(), + }, + run: (context) => { + expectExactType>(); + expectExactType>(); + expectExactType>(); + expectExactType>(); + + verbose = context.options.verbose; + output = context.options.output; + retries = context.options.retries; + files = context.options.files; + }, + }), + testInjector, + ); + + await command.execute([]); + + assert.isTrue(verbose); + assert.strictEqual(output, "dist"); + assert.strictEqual(retries, 3); + assert.deepEqual(files, ["a.ts"]); + }); + }); + + describe("dashedOptions", () => { + it("compiles the schema into the shape the option parser expects", () => { + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestdashed", + options: { + verbose: booleanOption({ default: false }), + output: stringOption({ alias: "o" }), + retries: numberOption({ default: 3 }), + files: arrayOption(), + token: stringOption({ + hasSensitiveValue: true, + description: "Auth token", + }), + }, + run: () => undefined, + }), + createTestInjector(), + ); + + assert.deepEqual(command.dashedOptions, { + verbose: { type: "boolean", hasSensitiveValue: false, default: false }, + output: { type: "string", hasSensitiveValue: false, alias: "o" }, + retries: { type: "number", hasSensitiveValue: false, default: 3 }, + files: { type: "array", hasSensitiveValue: false }, + token: { + type: "string", + hasSensitiveValue: true, + describe: "Auth token", + }, + }); + }); + + it("is empty when no options are declared", () => { + const command = createCommandFromDefinition( + defineCommand({ name: "dctestnoopts", run: () => undefined }), + createTestInjector(), + ); + + assert.deepEqual(command.dashedOptions, {}); + }); + }); + + describe("canExecute", () => { + it("is omitted for argument-less commands so the framework rejects parameters", () => { + const testInjector = createTestInjector(); + + const implicit = createCommandFromDefinition( + defineCommand({ name: "dctestnone", run: () => undefined }), + testInjector, + ); + const explicit = createCommandFromDefinition( + defineCommand({ + name: "dctestnone2", + arguments: "none", + run: () => undefined, + }), + testInjector, + ); + + assert.isUndefined(implicit.canExecute); + assert.isUndefined(explicit.canExecute); + assert.deepEqual(implicit.allowedParameters, []); + assert.deepEqual(explicit.allowedParameters, []); + }); + + it("accepts anything when arguments are 'any'", async () => { + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestany", + arguments: "any", + run: () => undefined, + }), + createTestInjector(), + ); + + assert.isFunction(command.canExecute); + assert.isTrue(await command.canExecute(["whatever", "else"])); + }); + + it("hands the context to a user canExecute and honours its verdict", async () => { + const testInjector = createTestInjector({ force: true }); + let capturedContext: any; + + const build = (verdict: boolean) => + createCommandFromDefinition( + defineCommand({ + name: "dctestverdict", + arguments: "any", + options: { force: booleanOption() }, + canExecute: (context) => { + capturedContext = context; + return verdict; + }, + run: () => undefined, + }), + testInjector, + ); + + assert.isTrue(await build(true).canExecute(["android"])); + assert.deepEqual(capturedContext.args, ["android"]); + assert.deepEqual(capturedContext.options, { force: true }); + + assert.isFalse(await build(false).canExecute(["android"])); + }); + + it("is emitted for a user canExecute even when arguments are 'none'", async () => { + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestnonecan", + arguments: "none", + canExecute: async () => true, + run: () => undefined, + }), + createTestInjector(), + ); + + assert.isFunction(command.canExecute); + assert.isTrue(await command.canExecute([])); + }); + }); + + describe("command flags", () => { + it("passes disableAnalytics and enableHooks through", () => { + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestflags", + disableAnalytics: true, + enableHooks: false, + run: () => undefined, + }), + createTestInjector(), + ); + + assert.isTrue(command.disableAnalytics); + assert.isFalse(command.enableHooks); + }); + + it("leaves both absent when the definition omits them", () => { + const command = createCommandFromDefinition( + defineCommand({ name: "dctestnoflags", run: () => undefined }), + createTestInjector(), + ); + + assert.isFalse("disableAnalytics" in command); + assert.isFalse("enableHooks" in command); + }); + }); + + describe("end to end through CommandsService", () => { + let validatedOptions: any; + + const createCommandsServiceInjector = (options: any): IInjector => { + const testInjector = new Yok(); + testInjector.register("errors", { + beginCommand: async (action: () => Promise) => action(), + failWithHelp: (message: string) => { + throw new Error(message); + }, + fail: (message: string) => { + throw new Error(message); + }, + }); + testInjector.register("hooksService", HooksServiceStub); + testInjector.register("logger", LoggerStub); + testInjector.register("staticConfig", { + disableAnalytics: true, + disableCommandHooks: true, + }); + testInjector.register("extensibilityService", {}); + testInjector.register("optionsTracker", {}); + testInjector.register("options", { + ...options, + validateOptions: (dashedOptions: any) => { + validatedOptions = dashedOptions; + }, + }); + testInjector.register("commandsService", CommandsService); + return testInjector; + }; + + beforeEach(() => { + validatedOptions = undefined; + }); + + it("validates the declared options and runs the command", async () => { + const testInjector = createCommandsServiceInjector({ verbose: true }); + let ran: any; + + registerCommandDefinition( + defineCommand({ + name: "dctest-e2e", + options: { verbose: booleanOption({ default: false }) }, + arguments: "any", + run: (context) => { + ran = context; + }, + }), + testInjector, + ); + + const commandsService: ICommandsService = + testInjector.resolve("commandsService"); + await commandsService.tryExecuteCommand("dctest-e2e", ["alpha"]); + + assert.deepEqual(validatedOptions, { + verbose: { type: "boolean", hasSensitiveValue: false, default: false }, + }); + assert.deepEqual(ran.args, ["alpha"]); + assert.deepEqual(ran.options, { verbose: true }); + }); + + it("lets the framework reject parameters when arguments are 'none'", async () => { + const testInjector = createCommandsServiceInjector({}); + let ran = false; + + registerCommandDefinition( + defineCommand({ + name: "dctest-e2e-none", + run: () => { + ran = true; + }, + }), + testInjector, + ); + + const commandsService: ICommandsService = + testInjector.resolve("commandsService"); + await assert.isRejected( + commandsService.tryExecuteCommand("dctest-e2e-none", ["stray"]), + /doesn't accept parameters/, + ); + assert.isFalse(ran); + }); + }); +}); From 15d95d26730aaeeaa8eaf9ee4c52c776996ccb62 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 22:07:33 -0300 Subject: [PATCH 02/18] fix(commands): don't resolve the options service for option-less definitions A definition with no declared options must be executable in a container that has no options service registered - manifest-loaded extension commands run in exactly that situation. --- lib/common/services/command-definition-adapter.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/common/services/command-definition-adapter.ts b/lib/common/services/command-definition-adapter.ts index 0956aa1db7..c56577775f 100644 --- a/lib/common/services/command-definition-adapter.ts +++ b/lib/common/services/command-definition-adapter.ts @@ -65,12 +65,16 @@ export function createCommandFromDefinition< // Resolved per call: the options service only holds parsed values once // validateOptions has run for this command. const buildContext = (args: string[]): ICommandContext => { - const optionsService: any = targetInjector.resolve("options"); const options: any = {}; - for (const optionName of optionNames) { - options[optionName] = optionsService - ? optionsService[optionName] - : undefined; + // Only a definition that declares options may depend on the options + // service being registered - a bare command must work without one. + if (optionNames.length) { + const optionsService: any = targetInjector.resolve("options"); + for (const optionName of optionNames) { + options[optionName] = optionsService + ? optionsService[optionName] + : undefined; + } } return { args, options }; From 7ee7b34764fb2ae63cf14ff13e7d0bb387e00722 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 22:39:13 -0300 Subject: [PATCH 03/18] test(commands): register hierarchical definitions on a local injector The parent-dispatcher leak onto the module-level injector is fixed in the base branch, so the round-trip test no longer needs the global facade. --- test/define-command.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/define-command.ts b/test/define-command.ts index 011ff59f31..da866802d2 100644 --- a/test/define-command.ts +++ b/test/define-command.ts @@ -1,5 +1,5 @@ import { assert } from "chai"; -import { Yok, injector as globalInjector } from "../lib/common/yok"; +import { Yok } from "../lib/common/yok"; import { IInjector } from "../lib/common/definitions/yok"; import { inject } from "../lib/common/di"; import { CommandsService } from "../lib/common/services/commands-service"; @@ -54,19 +54,18 @@ describe("defineCommand", () => { run: () => undefined, }); - // The parent dispatcher is synthesized onto the global facade by the - // legacy registry, so registration happens there too. - registerCommandDefinition(definition, globalInjector); + const testInjector = createTestInjector(); + registerCommandDefinition(definition, testInjector); - const command = globalInjector.resolveCommand("dctestwidget|add"); + const command = testInjector.resolveCommand("dctestwidget|add"); assert.isFunction(command.execute); assert.deepEqual(command.allowedParameters, []); - const parent = globalInjector.resolveCommand("dctestwidget"); + const parent = testInjector.resolveCommand("dctestwidget"); assert.isTrue(parent.isHierarchicalCommand); assert.include( - globalInjector.getRegisteredCommandsNames(false), + testInjector.getRegisteredCommandsNames(false), "dctestwidget|add", ); }); From 1c6f26013d10021ce9462e7ad13c3c924e83842d Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 22:44:15 -0300 Subject: [PATCH 04/18] refactor(commands): use the typed di bridge on IInjector --- lib/common/services/command-definition-adapter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/common/services/command-definition-adapter.ts b/lib/common/services/command-definition-adapter.ts index c56577775f..5b1c3da997 100644 --- a/lib/common/services/command-definition-adapter.ts +++ b/lib/common/services/command-definition-adapter.ts @@ -86,7 +86,7 @@ export function createCommandFromDefinition< execute: async (args: string[]): Promise => { // runInInjectionContext is synchronous, so inject() is available while // run() executes up to its first await, and not after it. - await runInInjectionContext((targetInjector).di, () => + await runInInjectionContext(targetInjector.di, () => definition.run(buildContext(args)), ); }, From e005adf73147bbf15240deef24d764f8f7f2113e Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 23:27:00 -0300 Subject: [PATCH 05/18] refactor(commands): the target injector IS the injection context Yok extends Injector on the base branch; the di bridge is gone. --- lib/common/services/command-definition-adapter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/common/services/command-definition-adapter.ts b/lib/common/services/command-definition-adapter.ts index 5b1c3da997..e92e5a88c8 100644 --- a/lib/common/services/command-definition-adapter.ts +++ b/lib/common/services/command-definition-adapter.ts @@ -86,7 +86,7 @@ export function createCommandFromDefinition< execute: async (args: string[]): Promise => { // runInInjectionContext is synchronous, so inject() is available while // run() executes up to its first await, and not after it. - await runInInjectionContext(targetInjector.di, () => + await runInInjectionContext(targetInjector, () => definition.run(buildContext(args)), ); }, From bca21ffd699042742b98d12c32f9b5df184f7ea9 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 15:50:01 -0300 Subject: [PATCH 06/18] feat(commands): validate definitions at define time and compose the argument policy Reworks the declarative command API after the design review: - the new public types drop the `I` prefix, and `defineCommand` returns a `DefinedCommand` branded with the marker `isCommandDefinition` narrows to. `registerCommandDefinition` requires that brand, so nothing reaches the registry without having been validated. - an option is `T` only when its spec declares a `default`; without one it is `T | undefined`, which is what the command line actually produces. Asserted by test/type-fixtures, compiled under strict mode because this build has strictNullChecks off. - `defineCommand` validates the definition and throws naming the command and the accepted form, instead of failing deep and unattributed later. - `arguments` is enforced before the definition's `canExecute` runs, so the two compose: a command that leaves `arguments` at "none" rejects stray positional arguments whether or not it refines further. - `canExecute` runs in an injection context, like `run`. - registration goes through the `CommandRegistry` facet the target injector provides rather than the injector itself. - a schema entry shadowing a CLI-wide option warns naming the collision. --- lib/common/define-command.ts | 318 ++++++++-- .../services/command-definition-adapter.ts | 203 ++++--- lib/contracts/index.ts | 10 +- test/define-command.ts | 542 ++++++++++++++++-- test/type-fixtures/define-command-types.ts | 82 +++ test/type-fixtures/tsconfig.json | 13 + tsconfig.json | 6 +- 7 files changed, 1002 insertions(+), 172 deletions(-) create mode 100644 test/type-fixtures/define-command-types.ts create mode 100644 test/type-fixtures/tsconfig.json diff --git a/lib/common/define-command.ts b/lib/common/define-command.ts index b197709ca9..53327119e6 100644 --- a/lib/common/define-command.ts +++ b/lib/common/define-command.ts @@ -8,15 +8,16 @@ /** * Symbol.for so that a definition produced by one copy of the CLI is still - * recognised by another — extensions bundle their own node_modules. + * recognised by another — extensions bundle their own node_modules. `unique + * symbol` so the marker can also be spelled in the branded return type. */ -export const COMMAND_DEFINITION_MARKER = Symbol.for( +export const COMMAND_DEFINITION_MARKER: unique symbol = Symbol.for( "nativescript:cli:commandDefinition", ); export type CommandOptionType = "boolean" | "string" | "number" | "array"; -export interface ICommandOptionSpec { +export interface CommandOptionSpec { type: CommandOptionType; /** Value used when the flag is absent from the command line. */ default?: TValue; @@ -24,81 +25,318 @@ export interface ICommandOptionSpec { alias?: string | string[]; /** Keeps the value out of analytics and logs. Defaults to false. */ hasSensitiveValue?: boolean; + /** Reserved for generated help; nothing renders it yet. */ description?: string; } +/** + * A spec whose `default` is required. The required property is what + * `CommandOptionValues` keys off to drop `| undefined` from the value type, so + * it may not be relaxed to an optional one. + */ +export interface DefaultedCommandOptionSpec< + TValue = any, +> extends CommandOptionSpec { + default: TValue; +} + /** The parts of an option spec a caller supplies; `type` comes from the helper. */ export type CommandOptionSpecInit = Omit< - ICommandOptionSpec, + CommandOptionSpec, "type" >; -export interface ICommandOptionsSchema { - [optionName: string]: ICommandOptionSpec; +export interface CommandOptionsSchema { + [optionName: string]: CommandOptionSpec; } -export type CommandOptionValues = { - [K in keyof TSchema]: TSchema[K] extends ICommandOptionSpec - ? TValue +/** + * An option the command line omitted is absent at runtime, so only a spec that + * declares a `default` yields a value that is always there. + */ +type CommandOptionValue = + TSpec extends CommandOptionSpec + ? TSpec extends { default: any } + ? TValue + : TValue | undefined : any; + +export type CommandOptionValues = { + [K in keyof TSchema]: CommandOptionValue; }; -export interface ICommandContext< - TSchema extends ICommandOptionsSchema = ICommandOptionsSchema, -> { +export interface CommandContext { /** Positional arguments, after the command name has been consumed. */ args: string[]; /** Current value of every option declared in the schema, and nothing else. */ options: CommandOptionValues; } -export interface ICommandDefinition< - TSchema extends ICommandOptionsSchema = ICommandOptionsSchema, -> { +export interface CommandDefinition { /** `"widget|add"`; `|` separates hierarchy levels. Several names alias one command. */ name: string | string[]; description?: string; options?: TSchema; /** * `"none"` (the default) rejects positional arguments; `"any"` accepts them. - * Anything finer belongs in `canExecute`. + * Anything finer belongs in `canExecute`, which runs after this policy. */ arguments?: "none" | "any"; - canExecute?(context: ICommandContext): Promise | boolean; + canExecute?(context: CommandContext): Promise | boolean; disableAnalytics?: boolean; enableHooks?: boolean; - run(context: ICommandContext): Promise | void; + run(context: CommandContext): Promise | void; +} + +/** + * What `defineCommand` returns: a definition carrying the marker in its type, + * so `registerCommandDefinition` can require a definition that went through + * define-time validation rather than any object of the right shape. + */ +export type DefinedCommand = + CommandDefinition & { + readonly [COMMAND_DEFINITION_MARKER]: true; + }; + +interface IOptionHelper { + ( + init: CommandOptionSpecInit & { default: TValue }, + ): DefaultedCommandOptionSpec; + (init?: CommandOptionSpecInit): CommandOptionSpec; } -const optionSpec = ( - type: CommandOptionType, - init: CommandOptionSpecInit, -): ICommandOptionSpec => ({ ...init, type }); +const optionHelper = (type: CommandOptionType): IOptionHelper => + >((init: CommandOptionSpecInit = {}) => ({ + ...init, + type, + })); + +export const booleanOption = optionHelper("boolean"); +export const stringOption = optionHelper("string"); +export const numberOption = optionHelper("number"); +export const arrayOption = optionHelper("array"); + +const DEFINITION_FIELDS = [ + "name", + "description", + "options", + "arguments", + "canExecute", + "disableAnalytics", + "enableHooks", + "run", +]; + +const OPTION_SPEC_FIELDS = [ + "type", + "default", + "alias", + "hasSensitiveValue", + "description", +]; + +const OPTION_TYPES: CommandOptionType[] = [ + "boolean", + "string", + "number", + "array", +]; + +const ACCEPTED_FORM = + 'defineCommand({ name: "widget|add", run(ctx) { ... } }) — with the ' + + "optional fields description, options, arguments, canExecute, " + + "disableAnalytics and enableHooks."; + +const describeDefinition = (definition: any): string => { + const name = definition && definition.name; + if (typeof name === "string" && name.length) { + return `'${name}'`; + } + + if (Array.isArray(name) && typeof name[0] === "string" && name[0].length) { + return `'${name[0]}'`; + } + + return "an unnamed command"; +}; + +const invalid = (definition: any, problem: string): never => { + throw new Error( + `Invalid command definition for ${describeDefinition(definition)}: ` + + `${problem}. Accepted form: ${ACCEPTED_FORM}`, + ); +}; + +const isPlainObject = (value: any): boolean => + !!value && typeof value === "object" && !Array.isArray(value); + +const validateName = (definition: any): void => { + const name = definition.name; + const isUsableName = (value: any) => + typeof value === "string" && value.trim().length > 0; + + if (isUsableName(name)) { + return; + } + + if (Array.isArray(name) && name.length && name.every(isUsableName)) { + return; + } + + invalid( + definition, + "'name' must be a non-empty string, or an array of non-empty strings for a command with aliases", + ); +}; + +const validateOptionSpec = ( + definition: any, + optionName: string, + spec: any, +): void => { + if (!isPlainObject(spec)) { + invalid( + definition, + `option '${optionName}' must be declared with one of booleanOption(), stringOption(), numberOption() or arrayOption()`, + ); + } + + if (OPTION_TYPES.indexOf(spec.type) === -1) { + invalid( + definition, + `option '${optionName}' has type '${spec.type}'; the supported types are ${OPTION_TYPES.join( + ", ", + )} — declare it with one of booleanOption(), stringOption(), numberOption() or arrayOption()`, + ); + } + + const unknownFields = Object.keys(spec).filter( + (field) => OPTION_SPEC_FIELDS.indexOf(field) === -1, + ); + if (unknownFields.length) { + invalid( + definition, + `option '${optionName}' has unknown field(s) ${unknownFields + .map((field) => `'${field}'`) + .join(", ")}; an option spec accepts ${OPTION_SPEC_FIELDS.join(", ")}`, + ); + } + + const aliasIsUsable = + spec.alias === undefined || + typeof spec.alias === "string" || + (Array.isArray(spec.alias) && + spec.alias.length > 0 && + spec.alias.every((entry: any) => typeof entry === "string")); + if (!aliasIsUsable) { + invalid( + definition, + `option '${optionName}' declares an 'alias' that is neither a string nor a non-empty array of strings`, + ); + } -export const booleanOption = ( - init: CommandOptionSpecInit = {}, -): ICommandOptionSpec => optionSpec("boolean", init); + if ( + spec.hasSensitiveValue !== undefined && + typeof spec.hasSensitiveValue !== "boolean" + ) { + invalid( + definition, + `option '${optionName}' declares a non-boolean 'hasSensitiveValue'`, + ); + } -export const stringOption = ( - init: CommandOptionSpecInit = {}, -): ICommandOptionSpec => optionSpec("string", init); + if (spec.description !== undefined && typeof spec.description !== "string") { + invalid( + definition, + `option '${optionName}' declares a non-string 'description'`, + ); + } +}; + +const validateDefinition = (definition: any): void => { + if (!isPlainObject(definition)) { + invalid(definition, "expected an object"); + } + + const unknownFields = Object.keys(definition).filter( + (field) => DEFINITION_FIELDS.indexOf(field) === -1, + ); + if (unknownFields.length) { + invalid( + definition, + `unknown field(s) ${unknownFields + .map((field) => `'${field}'`) + .join(", ")}; a definition accepts ${DEFINITION_FIELDS.join(", ")}`, + ); + } + + validateName(definition); + + if (typeof definition.run !== "function") { + invalid(definition, "'run' must be a function"); + } + + if ( + definition.arguments !== undefined && + definition.arguments !== "none" && + definition.arguments !== "any" + ) { + invalid( + definition, + `'arguments' is '${definition.arguments}'; it must be "none" or "any"`, + ); + } -export const numberOption = ( - init: CommandOptionSpecInit = {}, -): ICommandOptionSpec => optionSpec("number", init); + if ( + definition.canExecute !== undefined && + typeof definition.canExecute !== "function" + ) { + invalid(definition, "'canExecute' must be a function"); + } + + for (const flag of ["disableAnalytics", "enableHooks"]) { + if ( + definition[flag] !== undefined && + typeof definition[flag] !== "boolean" + ) { + invalid(definition, `'${flag}' must be a boolean`); + } + } + + if (definition.description !== undefined) { + if (typeof definition.description !== "string") { + invalid(definition, "'description' must be a string"); + } + } + + if (definition.options !== undefined) { + if (!isPlainObject(definition.options)) { + invalid( + definition, + "'options' must be an object keyed by the long option name", + ); + } + + for (const optionName of Object.keys(definition.options)) { + validateOptionSpec( + definition, + optionName, + definition.options[optionName], + ); + } + } +}; -export const arrayOption = ( - init: CommandOptionSpecInit = {}, -): ICommandOptionSpec => optionSpec("array", init); +export function defineCommand( + definition: CommandDefinition, +): DefinedCommand { + validateDefinition(definition); -export function defineCommand( - definition: ICommandDefinition, -): ICommandDefinition { - const marked: ICommandDefinition = { ...definition }; - (marked)[COMMAND_DEFINITION_MARKER] = true; + const marked: any = { ...definition }; + marked[COMMAND_DEFINITION_MARKER] = true; return marked; } -export function isCommandDefinition(value: any): value is ICommandDefinition { +export function isCommandDefinition(value: any): value is DefinedCommand { return !!value && (value)[COMMAND_DEFINITION_MARKER] === true; } diff --git a/lib/common/services/command-definition-adapter.ts b/lib/common/services/command-definition-adapter.ts index e92e5a88c8..e26fa5009f 100644 --- a/lib/common/services/command-definition-adapter.ts +++ b/lib/common/services/command-definition-adapter.ts @@ -4,11 +4,14 @@ import { runInInjectionContext } from "../di/inject"; import { IDictionary, IDashedOption } from "../declarations"; import { IInjector } from "../definitions/yok"; import { ICommand } from "../definitions/commands"; +import { CommandRegistry } from "../contracts/command-registry"; import { + CommandContext, + CommandDefinition, CommandOptionType, - ICommandContext, - ICommandDefinition, - ICommandOptionsSchema, + CommandOptionsSchema, + DefinedCommand, + isCommandDefinition, } from "../define-command"; const OPTION_TYPES: IDictionary = { @@ -18,29 +21,12 @@ const OPTION_TYPES: IDictionary = { array: OptionType.Array, }; -/** - * Wraps a declarative definition in the ICommand shape the legacy registry and - * CommandsService expect. - * - * Constraint worth knowing before changing the canExecute mapping: the moment - * an ICommand exposes `canExecute`, CommandsService returns its verdict and - * skips `allowedParameters` validation entirely. So `canExecute` is emitted - * only when the definition supplies one or opts into `arguments: "any"` — the - * adapter then owns argument validation wholesale. With `arguments: "none"` - * and no user `canExecute` the property is omitted, which lets the framework's - * own empty-`allowedParameters` check reject stray positional arguments. - */ -export function createCommandFromDefinition< - TSchema extends ICommandOptionsSchema, ->( - definition: ICommandDefinition, - targetInjector: IInjector = injector, -): ICommand { - const schema = definition.options || {}; - const optionNames = Object.keys(schema); - +const compileOptions = ( + schema: CommandOptionsSchema, +): IDictionary => { const dashedOptions: IDictionary = {}; - for (const optionName of optionNames) { + + for (const optionName of Object.keys(schema)) { const spec = schema[optionName]; const dashedOption: IDashedOption = { type: OPTION_TYPES[spec.type], @@ -62,63 +48,150 @@ export function createCommandFromDefinition< dashedOptions[optionName] = dashedOption; } - // Resolved per call: the options service only holds parsed values once - // validateOptions has run for this command. - const buildContext = (args: string[]): ICommandContext => { + return dashedOptions; +}; + +/** + * A command option that shadows a CLI-wide one wins the re-parse for this + * command only, so the two spellings mean different things depending on what + * the user typed first. Warned rather than rejected while the policy is open. + */ +const warnOnCliOptionCollisions = ( + targetInjector: IInjector, + definition: CommandDefinition, + optionNames: string[], + optionsService: any, +): void => { + const cliOptions = optionsService && optionsService.options; + if (!cliOptions) { + return; + } + + const collisions = optionNames.filter((optionName) => cliOptions[optionName]); + if (!collisions.length) { + return; + } + + const logger = targetInjector.get("logger", { optional: true }); + if (!logger) { + return; + } + + const commandName = Array.isArray(definition.name) + ? definition.name[0] + : definition.name; + logger.warn( + `Command '${commandName}' declares option(s) ${collisions + .map((name) => `'--${name}'`) + .join(", ")} that the CLI already defines globally. The command's ` + + `declaration wins while the command runs; rename them to avoid it.`, + ); +}; + +/** + * Wraps a declarative definition in the ICommand shape the legacy registry and + * CommandsService expect. + * + * The compiled command always exposes `canExecute`, because CommandsService + * skips `allowedParameters` entirely once it is present: the adapter enforces + * the declared `arguments` policy itself and only then consults the + * definition's own `canExecute`, so the two fields compose. + */ +export function createCommandFromDefinition< + TSchema extends CommandOptionsSchema, +>( + definition: CommandDefinition, + targetInjector: IInjector = injector, +): ICommand { + const schema = definition.options || {}; + const optionNames = Object.keys(schema); + const dashedOptions = compileOptions(schema); + + // Only a definition that declares options may depend on the options service + // being registered - a bare command must work without one. + const optionsService: any = optionNames.length + ? targetInjector.resolve("options") + : null; + + warnOnCliOptionCollisions( + targetInjector, + definition, + optionNames, + optionsService, + ); + + // Read per call rather than snapshotted here: the options service only holds + // this command's parsed values once validateOptions has run for it. + const buildContext = (args: string[]): CommandContext => { const options: any = {}; - // Only a definition that declares options may depend on the options - // service being registered - a bare command must work without one. - if (optionNames.length) { - const optionsService: any = targetInjector.resolve("options"); - for (const optionName of optionNames) { - options[optionName] = optionsService - ? optionsService[optionName] - : undefined; - } + for (const optionName of optionNames) { + options[optionName] = optionsService[optionName]; } return { args, options }; }; - const command: ICommand = { + const acceptsArguments = definition.arguments === "any"; + + return { allowedParameters: [], dashedOptions, + ...(definition.disableAnalytics === undefined + ? {} + : { disableAnalytics: definition.disableAnalytics }), + ...(definition.enableHooks === undefined + ? {} + : { enableHooks: definition.enableHooks }), + canExecute: async (args: string[]): Promise => { + if (!acceptsArguments && args.length) { + targetInjector + .resolve("errors") + .failWithHelp("This command doesn't accept parameters."); + return false; + } + + const refine = definition.canExecute; + if (!refine) { + return true; + } + + // Same first-await rule as execute: runInInjectionContext is + // synchronous, so inject() is available up to the first await. + return await runInInjectionContext(targetInjector, () => + refine.call(definition, buildContext(args)), + ); + }, execute: async (args: string[]): Promise => { - // runInInjectionContext is synchronous, so inject() is available while - // run() executes up to its first await, and not after it. await runInInjectionContext(targetInjector, () => definition.run(buildContext(args)), ); }, }; +} - if (definition.canExecute || definition.arguments === "any") { - command.canExecute = async (args: string[]): Promise => - definition.canExecute - ? await definition.canExecute(buildContext(args)) - : true; +export function registerCommandDefinition( + definition: DefinedCommand, + targetInjector: IInjector = injector, +): void { + if (!isCommandDefinition(definition)) { + throw new Error( + "registerCommandDefinition() takes the result of defineCommand(); " + + "the value passed carries no command-definition marker.", + ); } - if (definition.disableAnalytics !== undefined) { - command.disableAnalytics = definition.disableAnalytics; - } + // 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); + const names = Array.isArray(definition.name) + ? definition.name + : [definition.name]; - if (definition.enableHooks !== undefined) { - command.enableHooks = definition.enableHooks; + for (const name of names) { + // 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), + ); } - - return command; -} - -export function registerCommandDefinition< - TSchema extends ICommandOptionsSchema, ->( - definition: ICommandDefinition, - targetInjector: IInjector = injector, -): void { - // An arrow function resolver is invoked as a factory rather than new-ed, and - // its result is cached as the command instance. - targetInjector.registerCommand(definition.name, () => - createCommandFromDefinition(definition, targetInjector), - ); } diff --git a/lib/contracts/index.ts b/lib/contracts/index.ts index efbdc504f6..0120e5301a 100644 --- a/lib/contracts/index.ts +++ b/lib/contracts/index.ts @@ -35,10 +35,12 @@ export { arrayOption, } from "../common/define-command"; export type { - ICommandDefinition, - ICommandContext, - ICommandOptionSpec, - ICommandOptionsSchema, + CommandDefinition, + DefinedCommand, + CommandContext, + CommandOptionSpec, + DefaultedCommandOptionSpec, + CommandOptionsSchema, CommandOptionSpecInit, CommandOptionType, CommandOptionValues, diff --git a/test/define-command.ts b/test/define-command.ts index da866802d2..9bcf16a5b3 100644 --- a/test/define-command.ts +++ b/test/define-command.ts @@ -1,8 +1,13 @@ import { assert } from "chai"; +import { spawnSync } from "child_process"; +import * as path from "path"; import { Yok } from "../lib/common/yok"; import { IInjector } from "../lib/common/definitions/yok"; import { inject } from "../lib/common/di"; +import { CommandRegistry } from "../lib/common/contracts/command-registry"; import { CommandsService } from "../lib/common/services/commands-service"; +import { Options } from "../lib/options"; +import { Errors } from "../lib/common/errors"; import { LoggerStub, HooksServiceStub } from "./stubs"; import { arrayOption, @@ -17,17 +22,15 @@ import { registerCommandDefinition, } from "../lib/common/services/command-definition-adapter"; -type IsExact = - (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 - ? true - : false; - -/** Fails to compile unless the schema inferred exactly the expected type. */ -const expectExactType = (): void => undefined; - const createTestInjector = (options: any = {}): IInjector => { const testInjector = new Yok(); testInjector.register("options", options); + testInjector.register("logger", LoggerStub); + testInjector.register("errors", { + failWithHelp: (message: string) => { + throw new Error(message); + }, + }); return testInjector; }; @@ -35,7 +38,7 @@ describe("defineCommand", () => { it("marks definitions so a duplicated CLI copy still recognises them", () => { const definition = defineCommand({ name: "dctest-marker", - run: () => undefined, + run: (): void => undefined, }); assert.isTrue(isCommandDefinition(definition)); @@ -46,12 +49,195 @@ describe("defineCommand", () => { assert.isFalse(isCommandDefinition(null)); }); + it("keeps the marker on a spread-derived copy", () => { + const derived = { + ...defineCommand({ name: "dctest-spread", run: (): void => undefined }), + name: "dctest-spread-derived", + }; + + assert.isTrue(isCommandDefinition(derived)); + }); + + describe("define-time validation", () => { + const rejects = (definition: any, expected: RegExp) => + assert.throws(() => defineCommand(definition), expected); + + it("names the command and the accepted form in every message", () => { + rejects( + { name: "dctest-bad", run: 42 }, + /Invalid command definition for 'dctest-bad'.*'run' must be a function.*Accepted form: defineCommand/s, + ); + }); + + it("rejects a missing or unusable name", () => { + rejects( + { run: (): void => undefined }, + /an unnamed command.*'name' must be/s, + ); + rejects({ name: "", run: (): void => undefined }, /'name' must be/); + rejects({ name: [], run: (): void => undefined }, /'name' must be/); + rejects( + { name: ["ok", ""], run: (): void => undefined }, + /'name' must be/, + ); + rejects({ name: 7, run: (): void => undefined }, /'name' must be/); + }); + + it("rejects a missing run", () => { + rejects({ name: "dctest-norun" }, /'run' must be a function/); + }); + + it("rejects a typo'd definition field", () => { + rejects( + { + name: "dctest-typo", + handler: (): void => undefined, + run: (): void => undefined, + }, + /unknown field\(s\) 'handler'/, + ); + }); + + it("rejects an unusable arguments policy", () => { + rejects( + { name: "dctest-args", arguments: "one", run: (): void => undefined }, + /'arguments' is 'one'; it must be "none" or "any"/, + ); + }); + + it("rejects a non-function canExecute and non-boolean flags", () => { + rejects( + { name: "dctest-can", canExecute: true, run: (): void => undefined }, + /'canExecute' must be a function/, + ); + rejects( + { + name: "dctest-flag", + disableAnalytics: "yes", + run: (): void => undefined, + }, + /'disableAnalytics' must be a boolean/, + ); + rejects( + { name: "dctest-flag2", enableHooks: 1, run: (): void => undefined }, + /'enableHooks' must be a boolean/, + ); + }); + + it("rejects an option with an unsupported type", () => { + rejects( + { + name: "dctest-opt", + options: { verbose: { type: "bool" } }, + run: (): void => undefined, + }, + /option 'verbose' has type 'bool'; the supported types are boolean, string, number, array/, + ); + }); + + it("rejects an option that is not a spec at all", () => { + rejects( + { + name: "dctest-opt2", + options: { verbose: true }, + run: (): void => undefined, + }, + /option 'verbose' must be declared with one of booleanOption/, + ); + }); + + it("rejects a typo'd option-spec field", () => { + rejects( + { + name: "dctest-opt3", + options: { verbose: { type: "boolean", describe: "no" } }, + run: (): void => undefined, + }, + /option 'verbose' has unknown field\(s\) 'describe'/, + ); + }); + + it("rejects unusable alias, hasSensitiveValue and description entries", () => { + rejects( + { + name: "dctest-opt4", + options: { verbose: { type: "boolean", alias: 1 } }, + run: (): void => undefined, + }, + /option 'verbose' declares an 'alias'/, + ); + rejects( + { + name: "dctest-opt5", + options: { verbose: { type: "boolean", hasSensitiveValue: "yes" } }, + run: (): void => undefined, + }, + /non-boolean 'hasSensitiveValue'/, + ); + rejects( + { + name: "dctest-opt6", + options: { verbose: { type: "boolean", description: 5 } }, + run: (): void => undefined, + }, + /non-string 'description'/, + ); + }); + + it("accepts every documented field", () => { + assert.doesNotThrow(() => + defineCommand({ + name: ["dctest-full", "dctest-full-alias"], + description: "Everything at once", + options: { + verbose: booleanOption({ default: false }), + output: stringOption({ alias: ["o", "out"], description: "Dir" }), + retries: numberOption({ default: 1 }), + files: arrayOption({ hasSensitiveValue: true }), + }, + arguments: "any", + canExecute: () => true, + disableAnalytics: true, + enableHooks: false, + run: (): void => undefined, + }), + ); + }); + }); + + describe("option value types", () => { + it("types default-less options as possibly undefined", () => { + // The repo builds without strictNullChecks, which erases the very + // `| undefined` under test, so the assertions live in their own + // strict project. + const project = path.join( + __dirname, + "..", + "..", + "test", + "type-fixtures", + "tsconfig.json", + ); + const result = spawnSync( + process.execPath, + [require.resolve("typescript/bin/tsc"), "-p", project], + { encoding: "utf8" }, + ); + + assert.strictEqual( + result.status, + 0, + `${result.stdout || ""}${result.stderr || ""}`, + ); + }); + }); + describe("registration", () => { it("round-trips through the legacy command registry", () => { const definition = defineCommand({ name: "dctestwidget|add", description: "Adds a widget", - run: () => undefined, + run: (): void => undefined, }); const testInjector = createTestInjector(); @@ -73,7 +259,7 @@ describe("defineCommand", () => { it("caches one command instance per registered name", () => { const testInjector = createTestInjector(); registerCommandDefinition( - defineCommand({ name: "dctestflat", run: () => undefined }), + defineCommand({ name: "dctestflat", run: (): void => undefined }), testInjector, ); @@ -88,7 +274,7 @@ describe("defineCommand", () => { registerCommandDefinition( defineCommand({ name: ["dctestalias", "dctestalias2"], - run: () => undefined, + run: (): void => undefined, }), testInjector, ); @@ -96,6 +282,62 @@ describe("defineCommand", () => { assert.isFunction(testInjector.resolveCommand("dctestalias").execute); assert.isFunction(testInjector.resolveCommand("dctestalias2").execute); }); + + it("refuses a value that did not come from defineCommand", () => { + assert.throws( + () => + registerCommandDefinition( + { name: "dctestraw", run: (): void => undefined }, + createTestInjector(), + ), + /carries no command-definition marker/, + ); + }); + + it("registers through the CommandRegistry the target injector provides", () => { + const testInjector = createTestInjector(); + const registered: string[] = []; + testInjector.register({ + provide: CommandRegistry, + useValue: { + registerCommand: (name: string) => registered.push(name), + }, + }); + + registerCommandDefinition( + defineCommand({ + name: ["dctestfacet", "dctestfacet2"], + run: (): void => undefined, + }), + testInjector, + ); + + assert.deepEqual(registered, ["dctestfacet", "dctestfacet2"]); + assert.isNull(testInjector.resolveCommand("dctestfacet")); + }); + + it("keeps a registered command when a subcommand would shadow it", () => { + const testInjector = createTestInjector(); + registerCommandDefinition( + defineCommand({ name: "dctestowned", run: (): void => undefined }), + testInjector, + ); + + registerCommandDefinition( + defineCommand({ name: "dctestowned|sub", run: (): void => undefined }), + testInjector, + ); + + const owner = testInjector.resolveCommand("dctestowned"); + assert.isUndefined(owner.isHierarchicalCommand); + assert.isFunction(testInjector.resolveCommand("dctestowned|sub").execute); + + const logger: LoggerStub = testInjector.resolve("logger"); + assert.match( + logger.warnOutput, + /'dctestowned' is already registered as a command of its own.*'dctestowned\|sub' cannot be reached/, + ); + }); }); describe("execute", () => { @@ -176,7 +418,7 @@ describe("defineCommand", () => { assert.isTrue(finished); }); - it("infers the option value types on the run context", async () => { + it("carries the declared option values onto the run context", async () => { const testInjector = createTestInjector({ verbose: true, output: "dist", @@ -184,11 +426,7 @@ describe("defineCommand", () => { files: ["a.ts"], }); - let verbose: boolean; - let output: string; - let retries: number; - let files: string[]; - + let seen: any; const command = createCommandFromDefinition( defineCommand({ name: "dctesttypes", @@ -199,15 +437,7 @@ describe("defineCommand", () => { files: arrayOption(), }, run: (context) => { - expectExactType>(); - expectExactType>(); - expectExactType>(); - expectExactType>(); - - verbose = context.options.verbose; - output = context.options.output; - retries = context.options.retries; - files = context.options.files; + seen = context.options; }, }), testInjector, @@ -215,10 +445,12 @@ describe("defineCommand", () => { await command.execute([]); - assert.isTrue(verbose); - assert.strictEqual(output, "dist"); - assert.strictEqual(retries, 3); - assert.deepEqual(files, ["a.ts"]); + assert.deepEqual(seen, { + verbose: true, + output: "dist", + retries: 3, + files: ["a.ts"], + }); }); }); @@ -237,7 +469,7 @@ describe("defineCommand", () => { description: "Auth token", }), }, - run: () => undefined, + run: (): void => undefined, }), createTestInjector(), ); @@ -257,35 +489,75 @@ describe("defineCommand", () => { it("is empty when no options are declared", () => { const command = createCommandFromDefinition( - defineCommand({ name: "dctestnoopts", run: () => undefined }), + defineCommand({ name: "dctestnoopts", run: (): void => undefined }), createTestInjector(), ); assert.deepEqual(command.dashedOptions, {}); }); + + it("warns when a declared option shadows a CLI-wide one", () => { + const testInjector = createTestInjector({ + options: { verbose: { type: "boolean" } }, + }); + + createCommandFromDefinition( + defineCommand({ + name: "dctestshadow", + options: { verbose: booleanOption(), fresh: booleanOption() }, + run: (): void => undefined, + }), + testInjector, + ); + + const logger: LoggerStub = testInjector.resolve("logger"); + assert.match( + logger.warnOutput, + /Command 'dctestshadow' declares option\(s\) '--verbose' that the CLI already defines globally/, + ); + assert.notInclude(logger.warnOutput, "--fresh"); + }); }); describe("canExecute", () => { - it("is omitted for argument-less commands so the framework rejects parameters", () => { - const testInjector = createTestInjector(); + it("rejects positional arguments before consulting the definition", async () => { + let refined = false; + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestnone", + canExecute: () => { + refined = true; + return true; + }, + run: (): void => undefined, + }), + createTestInjector(), + ); - const implicit = createCommandFromDefinition( - defineCommand({ name: "dctestnone", run: () => undefined }), - testInjector, + await assert.isRejected( + command.canExecute(["stray"]), + /doesn't accept parameters/, ); - const explicit = createCommandFromDefinition( + assert.isFalse(refined); + assert.isTrue(await command.canExecute([])); + assert.isTrue(refined); + }); + + it("rejects positional arguments with no definition canExecute at all", async () => { + const command = createCommandFromDefinition( defineCommand({ name: "dctestnone2", arguments: "none", - run: () => undefined, + run: (): void => undefined, }), - testInjector, + createTestInjector(), ); - assert.isUndefined(implicit.canExecute); - assert.isUndefined(explicit.canExecute); - assert.deepEqual(implicit.allowedParameters, []); - assert.deepEqual(explicit.allowedParameters, []); + await assert.isRejected( + command.canExecute(["stray"]), + /doesn't accept parameters/, + ); + assert.isTrue(await command.canExecute([])); }); it("accepts anything when arguments are 'any'", async () => { @@ -293,16 +565,15 @@ describe("defineCommand", () => { defineCommand({ name: "dctestany", arguments: "any", - run: () => undefined, + run: (): void => undefined, }), createTestInjector(), ); - assert.isFunction(command.canExecute); assert.isTrue(await command.canExecute(["whatever", "else"])); }); - it("hands the context to a user canExecute and honours its verdict", async () => { + it("hands the context to a definition canExecute and honours its verdict", async () => { const testInjector = createTestInjector({ force: true }); let capturedContext: any; @@ -316,7 +587,7 @@ describe("defineCommand", () => { capturedContext = context; return verdict; }, - run: () => undefined, + run: (): void => undefined, }), testInjector, ); @@ -328,19 +599,21 @@ describe("defineCommand", () => { assert.isFalse(await build(false).canExecute(["android"])); }); - it("is emitted for a user canExecute even when arguments are 'none'", async () => { + it("runs the definition canExecute inside an injection context", async () => { + const testInjector = createTestInjector(); + testInjector.register("dcTestPolicy", { allowed: true }); + const command = createCommandFromDefinition( defineCommand({ - name: "dctestnonecan", - arguments: "none", - canExecute: async () => true, - run: () => undefined, + name: "dctestcaninject", + arguments: "any", + canExecute: () => inject("dcTestPolicy").allowed, + run: (): void => undefined, }), - createTestInjector(), + testInjector, ); - assert.isFunction(command.canExecute); - assert.isTrue(await command.canExecute([])); + assert.isTrue(await command.canExecute(["anything"])); }); }); @@ -351,7 +624,7 @@ describe("defineCommand", () => { name: "dctestflags", disableAnalytics: true, enableHooks: false, - run: () => undefined, + run: (): void => undefined, }), createTestInjector(), ); @@ -362,7 +635,7 @@ describe("defineCommand", () => { it("leaves both absent when the definition omits them", () => { const command = createCommandFromDefinition( - defineCommand({ name: "dctestnoflags", run: () => undefined }), + defineCommand({ name: "dctestnoflags", run: (): void => undefined }), createTestInjector(), ); @@ -371,10 +644,83 @@ describe("defineCommand", () => { }); }); + describe("option validation with the real options service", () => { + interface IValidationRun { + failures: string[]; + options: any; + } + + // The options service parses process.argv in its constructor, so each run + // gets its own injector and its own instance. + const validate = (definition: any, argv: string[]): IValidationRun => { + const failures: string[] = []; + const testInjector = new Yok(); + testInjector.register("staticConfig", { CLIENT_NAME: "" }); + testInjector.register("hostInfo", {}); + testInjector.register("settingsService", { + setSettings: (): any => undefined, + getProfileDir: () => "profileDir", + }); + testInjector.register("logger", LoggerStub); + + const errors = new Errors(testInjector); + errors.failWithHelp = ((message: string) => failures.push(message)); + errors.fail = ((message: string) => failures.push(message)); + testInjector.register("errors", errors); + testInjector.register("options", Options); + + const originalArgv = process.argv; + process.argv = [originalArgv[0], originalArgv[1], ...argv]; + try { + const command = createCommandFromDefinition(definition, testInjector); + const options: any = testInjector.resolve("options"); + options.validateOptions(command.dashedOptions); + return { failures, options }; + } finally { + process.argv = originalArgv; + } + }; + + beforeEach(() => { + process.env.NS_STRICT_OPTIONS = "error"; + }); + + afterEach(() => { + delete process.env.NS_STRICT_OPTIONS; + }); + + it("accepts an option declared with an array of aliases, under any spelling", () => { + const definition = defineCommand({ + name: "dctest-alias", + options: { outputDir: stringOption({ alias: ["o", "out"] }) }, + run: (): void => undefined, + }); + + for (const spelling of ["--output-dir", "--outputDir", "-o", "--out"]) { + const run = validate(definition, [spelling, "dist"]); + assert.deepEqual(run.failures, [], `rejected ${spelling}`); + assert.strictEqual(run.options.outputDir, "dist"); + } + }); + + it("still rejects an option the definition did not declare", () => { + const definition = defineCommand({ + name: "dctest-alias2", + options: { outputDir: stringOption({ alias: ["o", "out"] }) }, + run: (): void => undefined, + }); + + const run = validate(definition, ["--outputdirr", "dist"]); + + assert.lengthOf(run.failures, 1); + assert.match(run.failures[0], /'outputdirr' is not supported/); + }); + }); + describe("end to end through CommandsService", () => { let validatedOptions: any; - const createCommandsServiceInjector = (options: any): IInjector => { + const createCommandsServiceInjector = (options: any = {}): IInjector => { const testInjector = new Yok(); testInjector.register("errors", { beginCommand: async (action: () => Promise) => action(), @@ -434,8 +780,8 @@ describe("defineCommand", () => { assert.deepEqual(ran.options, { verbose: true }); }); - it("lets the framework reject parameters when arguments are 'none'", async () => { - const testInjector = createCommandsServiceInjector({}); + it("rejects parameters when arguments are 'none'", async () => { + const testInjector = createCommandsServiceInjector(); let ran = false; registerCommandDefinition( @@ -456,5 +802,77 @@ describe("defineCommand", () => { ); assert.isFalse(ran); }); + + it("rejects parameters even when the definition supplies a canExecute", async () => { + const testInjector = createCommandsServiceInjector(); + let ran = false; + + registerCommandDefinition( + defineCommand({ + name: "dctest-e2e-refine", + canExecute: () => true, + run: () => { + ran = true; + }, + }), + testInjector, + ); + + const commandsService: ICommandsService = + testInjector.resolve("commandsService"); + await assert.isRejected( + commandsService.tryExecuteCommand("dctest-e2e-refine", ["stray"]), + /doesn't accept parameters/, + ); + assert.isFalse(ran); + }); + + it("dispatches a subcommand through the parent name", async () => { + const testInjector = createCommandsServiceInjector(); + let ran: any; + + registerCommandDefinition( + defineCommand({ + name: "dctest-widget|add", + arguments: "any", + run: (context) => { + ran = context; + }, + }), + testInjector, + ); + + const commandsService: ICommandsService = + testInjector.resolve("commandsService"); + await commandsService.tryExecuteCommand("dctest-widget", [ + "add", + "alpha", + ]); + + assert.deepEqual(ran.args, ["alpha"]); + }); + + it("dispatches the default subcommand, named or bare", async () => { + const testInjector = createCommandsServiceInjector(); + const runs: string[][] = []; + + registerCommandDefinition( + defineCommand({ + name: "dctest-gadget|*all", + arguments: "any", + run: (context) => { + runs.push(context.args); + }, + }), + testInjector, + ); + + const commandsService: ICommandsService = + testInjector.resolve("commandsService"); + await commandsService.tryExecuteCommand("dctest-gadget", ["all", "beta"]); + await commandsService.tryExecuteCommand("dctest-gadget", []); + + assert.deepEqual(runs, [["beta"], []]); + }); }); }); diff --git a/test/type-fixtures/define-command-types.ts b/test/type-fixtures/define-command-types.ts new file mode 100644 index 0000000000..7bb50de7cb --- /dev/null +++ b/test/type-fixtures/define-command-types.ts @@ -0,0 +1,82 @@ +/** + * Type-level assertions for the defineCommand schema, compiled by + * test/define-command.ts through this directory's tsconfig. It is kept out of + * the repo's own build because that build runs without strictNullChecks, which + * erases the `| undefined` these assertions exist to pin — and because the + * @ts-expect-error directives below only hold under strict mode. + */ + +import { + arrayOption, + booleanOption, + defineCommand, + numberOption, + stringOption, +} from "../../lib/common/define-command"; + +type IsExact = + (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 + ? true + : false; + +const expectExactType = (): void => undefined; + +// A declared option is `T` only when the schema supplies a default; without +// one the flag may simply be absent from the command line. +defineCommand({ + name: "typefixture|values", + options: { + verbose: booleanOption(), + release: booleanOption({ default: false }), + output: stringOption({ alias: "o" }), + target: stringOption({ default: "dist" }), + retries: numberOption(), + attempts: numberOption({ default: 3 }), + files: arrayOption(), + tags: arrayOption({ default: [] }), + }, + run(ctx) { + expectExactType>(); + expectExactType>(); + expectExactType>(); + expectExactType>(); + expectExactType>(); + expectExactType>(); + expectExactType>(); + expectExactType>(); + expectExactType>(); + + // @ts-expect-error - the schema types ctx.options and nothing else + ctx.options.undeclared; + }, +}); + +defineCommand({ + name: "typefixture|no-options", + run(ctx) { + expectExactType>(); + + // @ts-expect-error - nothing is declared, so any access is a typo + ctx.options.anything; + }, +}); + +defineCommand({ + name: "typefixture|refine", + options: { force: booleanOption({ default: false }) }, + canExecute(ctx) { + expectExactType>(); + return ctx.args.length === 1; + }, + run: () => undefined, +}); + +// @ts-expect-error - `run` is the required handler field +defineCommand({ name: "typefixture|no-run" }); + +defineCommand({ + name: "typefixture|bad-arguments", + // @ts-expect-error - `arguments` is a closed set + arguments: "one", + run: () => undefined, +}); diff --git a/test/type-fixtures/tsconfig.json b/test/type-fixtures/tsconfig.json new file mode 100644 index 0000000000..254bd447c0 --- /dev/null +++ b/test/type-fixtures/tsconfig.json @@ -0,0 +1,13 @@ +{ + "compilerOptions": { + "target": "ES2018", + "module": "commonjs", + "noEmit": true, + "strict": true, + "skipLibCheck": true, + "noUnusedLocals": false, + "lib": ["ESNext"], + "types": [] + }, + "files": ["define-command-types.ts"] +} diff --git a/tsconfig.json b/tsconfig.json index 1e9b8deda9..45bf605cf7 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -18,5 +18,9 @@ "lib": ["ESNext"], "strict": false }, - "include": ["lib/", "test/"] + // test/type-fixtures has its own strict tsconfig and is compiled by the test + // that asserts on it; building it here would emit a testless file into dist + // and drop the strictness those assertions depend on + "include": ["lib/", "test/"], + "exclude": ["test/type-fixtures/"] } From 816082250836189ae34274c0f1c06a5a23d1aa52 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 15:50:01 -0300 Subject: [PATCH 07/18] docs(commands): match defining-commands.md to the shipped behavior Unknown options warn and only fail under NS_STRICT_OPTIONS=error; `description` reaches the parser but nothing renders it; `canExecute` gets a context of the same shape as run's, not the same one. Replaces the "canExecute owns validation" rule with how the two fields compose, renames the flagship example's option off the CLI-wide `verbose`, and documents option value types, array aliases, the parent-name collision and `satisfies` for shared schemas. --- defining-commands.md | 159 +++++++++++++++++++++++++++++++------------ 1 file changed, 114 insertions(+), 45 deletions(-) diff --git a/defining-commands.md b/defining-commands.md index fa0143123b..cfadf9cd27 100644 --- a/defining-commands.md +++ b/defining-commands.md @@ -26,25 +26,45 @@ export default defineCommand({ name: "widget|add", description: "Adds a widget to the project", options: { - verbose: booleanOption({ default: false }), + overwrite: booleanOption({ default: false }), output: stringOption({ alias: "o" }), }, arguments: "any", async run(ctx) { // ctx.args -> string[] of positional arguments - // ctx.options -> { verbose: boolean; output: string } - if (ctx.options.verbose) { + // ctx.options -> { overwrite: boolean; output: string | undefined } + if (ctx.options.output) { console.log(`adding ${ctx.args.join(", ")} to ${ctx.options.output}`); } }, }); ``` -`defineCommand` returns the definition, tagged with a marker symbol so that any -copy of the CLI can recognise it (`isCommandDefinition(value)` is the exported -predicate). It does not register anything by itself — see +`defineCommand` validates the definition and returns it, tagged with a marker +symbol so that any copy of the CLI can recognise it. `isCommandDefinition(value)` +is the exported check, and it narrows to `DefinedCommand`. The tag survives a +spread, so `{ ...baseDefinition, name: "widget|add2" }` is still recognised. + +`defineCommand` does not register anything by itself — see [Registering a definition](#registering-a-definition). +Validation happens where you can see it +--------------------------------------- + +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: + +``` +Invalid command definition for 'widget|add': unknown field(s) 'handler'; a +definition accepts name, description, options, arguments, canExecute, +disableAnalytics, enableHooks, run. Accepted form: defineCommand({ name: +"widget|add", run(ctx) { ... } }) — with the optional fields description, +options, arguments, canExecute, disableAnalytics and enableHooks. +``` + Names and the command hierarchy ------------------------------- @@ -62,19 +82,29 @@ runs both for `ns widget add` and for a bare `ns widget`. This is the convention the CLI's own commands use (`run|*all`, `debug|*all`); the encoding is user-visible because it feeds shell autocompletion and generated help. +A parent name cannot also be a command of its own. If `widget` is already +registered as a flat command, registering `widget|add` leaves that command in +place, warns naming both, and creates no dispatcher — so `ns widget add` will +not route until one of the two is renamed. + Options ------- -`options` is a schema keyed by the long option name — `verbose` is passed as -`--verbose`. Declare each entry with one of the four helpers, which fix the +`options` is a schema keyed by the long option name — `output` is passed as +`--output`. Declare each entry with one of the four helpers, which fix the value type: -| Helper | Value type on `ctx.options` | -| --------------- | --------------------------- | -| `booleanOption` | `boolean` | -| `stringOption` | `string` | -| `numberOption` | `number` | -| `arrayOption` | `string[]` | +| Helper | Declared with `default` | Declared without | +| --------------- | ----------------------- | ----------------------- | +| `booleanOption` | `boolean` | `boolean \| undefined` | +| `stringOption` | `string` | `string \| undefined` | +| `numberOption` | `number` | `number \| undefined` | +| `arrayOption` | `string[]` | `string[] \| undefined` | + +The two columns are the whole story of the option types: a flag the user did +not pass is absent at runtime, so only a `default` makes the value on +`ctx.options` always present. Declare a default whenever there is a sensible +one and the `| undefined` disappears from the type. Each helper takes an optional spec: @@ -94,17 +124,39 @@ options: { ``` - `default` — value used when the flag is absent. -- `alias` — single-dash shorthand, or an array of them. +- `alias` — single-dash shorthand, or an array of them (`alias: ["o", "out"]`). - `hasSensitiveValue` — defaults to `false`; set it for anything that must not be recorded. There is no reason not to be explicit about credentials, paths containing user directories, and tokens. -- `description` — shown in generated help. +- `description` — reserved for generated help. It reaches the option parser but + nothing renders it yet. The schema types `ctx.options` and nothing else: `ctx.options` carries exactly -the declared keys, with the types the helpers imply. Values that the CLI parses +the declared keys, and a typo is a compile error. Values that the CLI parses globally (`--path`, `--log`, …) are not exposed there; resolve the `options` service if you need them. +### Sharing a schema between commands + +Extract the schema with `satisfies` rather than a type annotation. An +annotation widens every entry back to the general spec type and the `default` +information — and with it the non-optional value types — is lost: + +```ts +const buildOptions = { + release: booleanOption({ default: false }), + output: stringOption({ alias: "o" }), +} satisfies CommandOptionsSchema; +``` + +### Do not shadow a CLI-wide option + +`--verbose`, `--path`, `--log`, `--release`, `--env` and friends are declared by +the CLI itself. Declaring one of those names in a command's schema makes the +command's declaration win for the duration of that command, which means the +same flag means different things depending on which command is running. The CLI +warns at registration naming the collision; pick another name. + ### How validation behaves Option validation is the CLI's existing behaviour, not something the definition @@ -113,11 +165,17 @@ declared options and the command line is re-parsed: - Declared options are accepted and appear on `ctx.options`. - An option the CLI does not know — neither global nor declared by this command - — is a **hard error**: the command does not run, and the CLI prints - `The option '' is not supported.` followed by a help suggestion. - -So adding an option is exactly a matter of adding a schema entry; forgetting to -declare one that users pass is a failure, not a silent `undefined`. + — produces a warning: `The option '' is not supported. This will become +an error in a future release.` The command still runs. Set + `NS_STRICT_OPTIONS=error` to preview the hard failure, which is what a future + release will do by default. +- The same staging applies to value-shape violations: a string option passed + with no value, an array option passed nothing, a single-valued option passed + twice. + +So adding an option is a matter of adding a schema entry; forgetting to declare +one that users pass is a warning today and a failure later, never a silent +`undefined`. Positional arguments -------------------- @@ -131,7 +189,7 @@ Positional arguments Anything finer than that belongs in `canExecute`. -### `canExecute` owns validation +### `canExecute` refines, it does not replace ```ts defineCommand({ @@ -146,18 +204,20 @@ defineCommand({ }); ``` -`canExecute` receives the same context as `run` and returns a boolean (or a -promise of one). Returning `false` aborts the command and prints a help -suggestion; throwing surfaces your own error message, which is usually the -friendlier choice. +The two fields compose. The declared `arguments` policy is enforced first, and +`canExecute` is consulted only for command lines that already satisfy it — so a +definition that leaves `arguments` at `"none"` still rejects stray positional +arguments even when it supplies a `canExecute`, and a `canExecute` that only +inspects options cannot accidentally widen what the command accepts. + +`canExecute` receives a context of the same shape as `run`'s — the same +`args` and the same declared options, built freshly for the call — and returns +a boolean (or a promise of one). Returning `false` aborts the command and +prints a help suggestion; throwing surfaces your own error message, which is +usually the friendlier choice. -There is one rule to internalise: **the moment a command supplies -`canExecute`, it owns argument validation completely.** The framework returns -that verdict and skips every built-in parameter check, including the -"no parameters" rule implied by `arguments: "none"`. A definition with a -`canExecute` that only inspects options will therefore accept stray positional -arguments unless it checks `ctx.args` itself. If you do not need custom -validation, omit `canExecute` and let `arguments` do the work. +`canExecute` runs inside a dependency-injection context, on the same terms as +`run`: `inject()` is valid up to the first `await`. The run context --------------- @@ -170,7 +230,9 @@ The run context the command executes. `run` may be synchronous or `async`; the CLI awaits the result and treats a -rejection as a command failure. +rejection as a command failure. Throwing is how a definition fails a command; +`$errors.failWithHelp` from the injected `errors` service adds the help +suggestion. `run` starts inside a dependency-injection context, so `inject()` works directly: @@ -215,9 +277,12 @@ import addWidgetCommand from "./add-widget"; registerCommandDefinition(addWidgetCommand); ``` -It registers the definition under every name it declares, on the CLI's injector -by default; pass a second argument to target a different injector (tests do -this). The command instance is created on first resolution and cached. +It takes a `DefinedCommand` — the result of `defineCommand`, marker and all — +and rejects a bare object of the right shape, so a definition can never reach +the registry without having been validated. It registers under every name the +definition declares, through the `CommandRegistry` the target injector provides; +pass a second argument to target a different injector (tests do this). The +command instance is built by a factory on first resolution and cached. `registerCommandDefinition` lives in `lib/common/services/command-definition-adapter` rather than in @@ -237,13 +302,17 @@ A definition is compiled into an ordinary `ICommand`, so nothing downstream — the registry, the router, hooks, help, analytics — knows the difference. The mapping is: -| Definition | `ICommand` | -| --------------------------------- | ------------------------------------------ | -| `options` | `dashedOptions` | -| `run` | `execute`, wrapped in an injection context | -| `arguments`, `canExecute` | `canExecute` (see the rule above) | -| — | `allowedParameters`, always `[]` | -| `disableAnalytics`, `enableHooks` | passed through unchanged | +| Definition | `ICommand` | +| --------------------------------- | -------------------------------------------------- | +| `options` | `dashedOptions` | +| `run` | `execute`, wrapped in an injection context | +| `arguments`, `canExecute` | `canExecute`: policy enforced, then the refinement | +| — | `allowedParameters`, always `[]` | +| `disableAnalytics`, `enableHooks` | passed through unchanged | + +The compiled command always exposes `canExecute`, because `CommandsService` +stops consulting `allowedParameters` as soon as a command has one — the adapter +therefore enforces the `arguments` policy itself. Existing command classes need no migration. Reach for a definition when a command is mostly "parse these flags and do this"; a class still makes sense From d712a4e545f51d360a711a514871d0319c3fd3c0 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 16:18:14 -0300 Subject: [PATCH 08/18] feat(commands): add ctx.fail and warn on colliding option aliases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ctx.fail(message)` is the failure verb on the command context, in both `run` and `canExecute`. It maps to the errors service's `failWithHelp`, so a command failure carries the usage suggestion, and returns `never` so it can end a branch without a return. The message is validated like the define-time errors are, naming the command. Throwing keeps working unchanged — fail() is sugar over it, not a replacement. Commands get no `skip()`: warn-and-continue has no meaning inside run(). The CLI-wide option collision warning now covers aliases on both sides, so an `alias: "p"` that shadows `--path`'s shorthand is reported the same way a `verbose` option shadowing `--verbose` is, naming both sides. --- defining-commands.md | 47 +++++-- lib/common/define-command.ts | 2 + .../services/command-definition-adapter.ts | 74 ++++++++--- test/define-command.ts | 117 +++++++++++++++++- test/type-fixtures/define-command-types.ts | 2 + 5 files changed, 209 insertions(+), 33 deletions(-) diff --git a/defining-commands.md b/defining-commands.md index cfadf9cd27..b6f589cf9a 100644 --- a/defining-commands.md +++ b/defining-commands.md @@ -155,7 +155,11 @@ const buildOptions = { the CLI itself. Declaring one of those names in a command's schema makes the command's declaration win for the duration of that command, which means the same flag means different things depending on which command is running. The CLI -warns at registration naming the collision; pick another name. +warns at registration naming both sides of the collision; pick another name. + +Aliases count too, in both directions: an `alias: "p"` collides with `--path`'s +shorthand just as `output: stringOption()` would collide with a CLI-wide +`--output`. ### How validation behaves @@ -211,10 +215,10 @@ arguments even when it supplies a `canExecute`, and a `canExecute` that only inspects options cannot accidentally widen what the command accepts. `canExecute` receives a context of the same shape as `run`'s — the same -`args` and the same declared options, built freshly for the call — and returns -a boolean (or a promise of one). Returning `false` aborts the command and -prints a help suggestion; throwing surfaces your own error message, which is -usually the friendlier choice. +`args`, the same declared options and the same `fail` — built freshly for the +call, and returns a boolean (or a promise of one). Returning `false` aborts the +command and prints a bare help suggestion; `ctx.fail(message)` aborts it with +your own message, which is usually the friendlier choice. `canExecute` runs inside a dependency-injection context, on the same terms as `run`: `inject()` is valid up to the first `await`. @@ -228,11 +232,38 @@ The run context (including any subcommand segments) has been consumed. - `ctx.options` — the current value of each declared option, read at the moment the command executes. +- `ctx.fail(message)` — fails the command with `message` and a usage help + suggestion. `run` may be synchronous or `async`; the CLI awaits the result and treats a -rejection as a command failure. Throwing is how a definition fails a command; -`$errors.failWithHelp` from the injected `errors` service adds the help -suggestion. +rejection as a command failure. + +### Failing a command + +`ctx.fail(message)` is the idiomatic way to stop a command: + +```ts +defineCommand({ + name: "widget|add", + arguments: "any", + options: { output: stringOption() }, + async run(ctx) { + if (!ctx.options.output) { + ctx.fail("--output is required."); + } + + /* ... */ + }, +}); +``` + +It is available on the `canExecute` context as well, and it returns `never`, so +it can end a branch without a `return`. The message must be a non-empty string. + +Throwing is equivalent and keeps working — `ctx.fail` is sugar over the +`errors` service's `failWithHelp`, which is what adds the "Run `ns widget add +--help`" line. Throw when you already have an `Error` to propagate; call +`ctx.fail` when you are writing the message. `run` starts inside a dependency-injection context, so `inject()` works directly: diff --git a/lib/common/define-command.ts b/lib/common/define-command.ts index 53327119e6..c5cf28e5fe 100644 --- a/lib/common/define-command.ts +++ b/lib/common/define-command.ts @@ -70,6 +70,8 @@ export interface CommandContext { args: string[]; /** Current value of every option declared in the schema, and nothing else. */ options: CommandOptionValues; + /** Fails the command with `message` and the usage help suggestion. */ + fail(message: string): never; } export interface CommandDefinition { diff --git a/lib/common/services/command-definition-adapter.ts b/lib/common/services/command-definition-adapter.ts index e26fa5009f..92c85429b1 100644 --- a/lib/common/services/command-definition-adapter.ts +++ b/lib/common/services/command-definition-adapter.ts @@ -1,7 +1,7 @@ import { OptionType } from "../enums"; import { injector } from "../yok"; import { runInInjectionContext } from "../di/inject"; -import { IDictionary, IDashedOption } from "../declarations"; +import { IDictionary, IDashedOption, IErrors } from "../declarations"; import { IInjector } from "../definitions/yok"; import { ICommand } from "../definitions/commands"; import { CommandRegistry } from "../contracts/command-registry"; @@ -51,15 +51,18 @@ const compileOptions = ( return dashedOptions; }; +const aliasList = (alias: string | string[]): string[] => + alias === undefined ? [] : Array.isArray(alias) ? alias : [alias]; + /** * A command option that shadows a CLI-wide one wins the re-parse for this - * command only, so the two spellings mean different things depending on what - * the user typed first. Warned rather than rejected while the policy is open. + * command only, so the same spelling means different things depending on which + * command is running. Warned rather than rejected while the policy is open. */ const warnOnCliOptionCollisions = ( targetInjector: IInjector, definition: CommandDefinition, - optionNames: string[], + schema: CommandOptionsSchema, optionsService: any, ): void => { const cliOptions = optionsService && optionsService.options; @@ -67,7 +70,32 @@ const warnOnCliOptionCollisions = ( return; } - const collisions = optionNames.filter((optionName) => cliOptions[optionName]); + // Every spelling the CLI already answers to, mapped to the option owning it. + const cliSpellings: IDictionary = {}; + for (const cliName of Object.keys(cliOptions)) { + cliSpellings[cliName] = cliName; + for (const alias of aliasList(cliOptions[cliName].alias)) { + cliSpellings[alias] = cliName; + } + } + + const collisions: string[] = []; + for (const optionName of Object.keys(schema)) { + if (cliSpellings[optionName]) { + collisions.push( + `'--${optionName}' with the CLI option '--${cliSpellings[optionName]}'`, + ); + } + + for (const alias of aliasList(schema[optionName].alias)) { + if (cliSpellings[alias]) { + collisions.push( + `alias '-${alias}' of '--${optionName}' with the CLI option '--${cliSpellings[alias]}'`, + ); + } + } + } + if (!collisions.length) { return; } @@ -81,10 +109,9 @@ const warnOnCliOptionCollisions = ( ? definition.name[0] : definition.name; logger.warn( - `Command '${commandName}' declares option(s) ${collisions - .map((name) => `'--${name}'`) - .join(", ")} that the CLI already defines globally. The command's ` + - `declaration wins while the command runs; rename them to avoid it.`, + `Command '${commandName}' declares options that collide with CLI-wide ` + + `ones: ${collisions.join("; ")}. The command's declaration wins while ` + + `the command runs; rename them to avoid it.`, ); }; @@ -113,12 +140,22 @@ export function createCommandFromDefinition< ? targetInjector.resolve("options") : null; - warnOnCliOptionCollisions( - targetInjector, - definition, - optionNames, - optionsService, - ); + warnOnCliOptionCollisions(targetInjector, definition, schema, optionsService); + + const commandName = Array.isArray(definition.name) + ? definition.name[0] + : definition.name; + + const fail = (message: string): never => { + if (typeof message !== "string" || !message.trim()) { + throw new Error( + `ctx.fail() for command '${commandName}' requires a non-empty message.`, + ); + } + + const errors: IErrors = targetInjector.resolve("errors"); + return errors.failWithHelp(message); + }; // Read per call rather than snapshotted here: the options service only holds // this command's parsed values once validateOptions has run for it. @@ -128,7 +165,7 @@ export function createCommandFromDefinition< options[optionName] = optionsService[optionName]; } - return { args, options }; + return { args, options, fail }; }; const acceptsArguments = definition.arguments === "any"; @@ -144,10 +181,7 @@ export function createCommandFromDefinition< : { enableHooks: definition.enableHooks }), canExecute: async (args: string[]): Promise => { if (!acceptsArguments && args.length) { - targetInjector - .resolve("errors") - .failWithHelp("This command doesn't accept parameters."); - return false; + fail("This command doesn't accept parameters."); } const refine = definition.canExecute; diff --git a/test/define-command.ts b/test/define-command.ts index 9bcf16a5b3..f249669849 100644 --- a/test/define-command.ts +++ b/test/define-command.ts @@ -496,26 +496,58 @@ describe("defineCommand", () => { assert.deepEqual(command.dashedOptions, {}); }); - it("warns when a declared option shadows a CLI-wide one", () => { + it("warns when a declared option or alias shadows a CLI-wide one", () => { const testInjector = createTestInjector({ - options: { verbose: { type: "boolean" } }, + options: { + verbose: { type: "boolean" }, + path: { type: "string", alias: "p" }, + }, }); createCommandFromDefinition( defineCommand({ name: "dctestshadow", - options: { verbose: booleanOption(), fresh: booleanOption() }, + options: { + verbose: booleanOption(), + output: stringOption({ alias: ["p", "o"] }), + fresh: booleanOption({ alias: "f" }), + }, run: (): void => undefined, }), testInjector, ); const logger: LoggerStub = testInjector.resolve("logger"); - assert.match( + assert.include( + logger.warnOutput, + "'--verbose' with the CLI option '--verbose'", + ); + assert.include( logger.warnOutput, - /Command 'dctestshadow' declares option\(s\) '--verbose' that the CLI already defines globally/, + "alias '-p' of '--output' with the CLI option '--path'", ); assert.notInclude(logger.warnOutput, "--fresh"); + assert.notInclude(logger.warnOutput, "'-o'"); + }); + + it("stays quiet when nothing collides", () => { + const testInjector = createTestInjector({ + options: { path: { type: "string", alias: "p" } }, + }); + + createCommandFromDefinition( + defineCommand({ + name: "dctestnoshadow", + options: { output: stringOption({ alias: ["o", "out"] }) }, + run: (): void => undefined, + }), + testInjector, + ); + + assert.strictEqual( + (testInjector.resolve("logger")).warnOutput, + "", + ); }); }); @@ -617,6 +649,81 @@ describe("defineCommand", () => { }); }); + describe("ctx.fail", () => { + const createFailInjector = (): IInjector => { + const testInjector = createTestInjector(); + testInjector.register("errors", { + failWithHelp: (message: string) => { + throw new Error(`with help: ${message}`); + }, + }); + return testInjector; + }; + + it("fails the command from run, through failWithHelp", async () => { + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestfailrun", + run: (ctx) => ctx.fail("no project found"), + }), + createFailInjector(), + ); + + await assert.isRejected( + command.execute([]), + /with help: no project found/, + ); + }); + + it("fails the command from canExecute, through failWithHelp", async () => { + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestfailcan", + arguments: "any", + canExecute: (ctx) => + ctx.args.length === 1 || ctx.fail("expected one argument"), + run: (): void => undefined, + }), + createFailInjector(), + ); + + assert.isTrue(await command.canExecute(["one"])); + await assert.isRejected( + command.canExecute([]), + /with help: expected one argument/, + ); + }); + + it("rejects a message that carries nothing", async () => { + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestfailempty", + run: (ctx) => ctx.fail(" "), + }), + createFailInjector(), + ); + + await assert.isRejected( + command.execute([]), + /ctx.fail\(\) for command 'dctestfailempty' requires a non-empty message/, + ); + }); + + it("still lets a thrown error through unchanged", async () => { + const command = createCommandFromDefinition( + defineCommand({ + name: "dctestthrow", + run: () => { + throw new Error("raw failure"); + }, + }), + createFailInjector(), + ); + + await assert.isRejected(command.execute([]), /^raw failure$/); + }); + }); + describe("command flags", () => { it("passes disableAnalytics and enableHooks through", () => { const command = createCommandFromDefinition( diff --git a/test/type-fixtures/define-command-types.ts b/test/type-fixtures/define-command-types.ts index 7bb50de7cb..e8d1f80d49 100644 --- a/test/type-fixtures/define-command-types.ts +++ b/test/type-fixtures/define-command-types.ts @@ -55,6 +55,8 @@ defineCommand({ name: "typefixture|no-options", run(ctx) { expectExactType>(); + // `never` is what lets fail() end a branch without a return. + expectExactType, never>>(); // @ts-expect-error - nothing is declared, so any access is a typo ctx.options.anything; From 3c77ebb71c0c8b175fd65a7e028ad1171c58c734 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 21:59:53 -0300 Subject: [PATCH 09/18] feat(extensions): lazy per-command loading via a nativescript.commands map An extension whose package.json declares nativescript.commands as a map of command name to module path is no longer require()d at startup. Each entry is registered with injector.requireCommand against the module's absolute path, so a command's implementation loads only when that command is first resolved, and the CLI stops paying every installed extension's load cost on every invocation. Entries are validated: a command name or module path that is not a non-empty string is warned about and skipped, and a name already claimed by another extension is reported as a warning naming both extensions rather than propagating the injector's "require'd twice" failure. The legacy array shape (and a missing commands key) keeps today's behavior verbatim - eager require of the extension main plus the extensions.require-time-registration deprecation report. Both shapes now feed IExtensionData.commands and the npm install suggestion for unknown commands. --- extensions.md | 179 +++++++++ lib/common/definitions/extensibility.d.ts | 7 + lib/services/extensibility-service.ts | 144 ++++++- test/extension-manifests.ts | 460 ++++++++++++++++++++++ 4 files changed, 780 insertions(+), 10 deletions(-) create mode 100644 extensions.md create mode 100644 test/extension-manifests.ts diff --git a/extensions.md b/extensions.md new file mode 100644 index 0000000000..f92c00f1dc --- /dev/null +++ b/extensions.md @@ -0,0 +1,179 @@ +Writing a CLI Extension +======================= + +An extension adds new commands to the NativeScript CLI. Extensions are ordinary +npm packages: they are published to npm, installed per user rather than per +project, and are available from every project on the machine. + +```bash +ns extension install +ns extension uninstall +``` + +Installed extensions live in the CLI profile directory, under +`extensions/node_modules/`, and every CLI invocation consults each +of them. That makes the manifest below the most important file in an extension: +it is what the CLI reads on startup, and it decides whether your code is loaded +eagerly or only when one of your commands is actually executed. + +## Making a package discoverable + +Add the `nativescript:extension` keyword to `package.json`. The CLI searches npm +for that keyword when it needs to suggest an extension for an unknown command +(see [Suggesting an extension](#suggesting-an-extension-for-an-unknown-command)). + +```json +{ + "name": "nativescript-hello", + "version": "1.0.0", + "keywords": ["nativescript:extension"] +} +``` + +## Declaring commands + +Commands are declared in the `commands` key of the `nativescript` key of the +extension's `package.json`. Two shapes are accepted. + +### A map of command name to module (recommended) + +```json +{ + "nativescript": { + "commands": { + "hello|world": "./dist/commands/hello-world.js", + "hello|*default": "./dist/commands/hello.js" + } + } +} +``` + +Each key is a command name; each value is a path to the module implementing it, +resolved relative to the extension's root directory. + +Declaring commands this way is strongly preferred: + +* **Per-command lazy loading.** Nothing in the extension is loaded when the CLI + starts. A command's module is required the first time that command is + resolved, so `ns build android` never pays the cost of loading an unrelated + extension. With a large or dependency-heavy extension installed, that is the + difference between a noticeable startup delay on every command and none. +* **Early, named conflict detection.** Two extensions claiming the same command + name is reported as a warning that names both extensions and the contested + command, and the extension that claimed it first keeps working. Under the + legacy shape the same collision surfaces as an opaque + `module '...' require'd twice.` failure from whichever extension happened to + load second. +* **The CLI knows what you contribute without running you.** The declared + command names are what the install suggestion for an unknown command matches + against, and they are available to the CLI as metadata about the installed + extension. + +Malformed entries are skipped rather than fatal: an entry whose command name or +module path is not a non-empty string is reported as a warning naming the +extension and the offending entry, and the extension's remaining commands are +still registered. + +### An array of command names (legacy) + +```json +{ + "nativescript": { + "commands": ["hello|world", "hello|*default"] + } +} +``` + +The array is a discovery aid only — it lists the names the CLI may suggest your +extension for, but it says nothing about where the implementations live. An +extension declaring commands this way (or declaring no commands at all) is +loaded the old way: the CLI `require()`s the package's main entry on **every** +invocation and expects the module's top-level code to register everything. + +```js +// index.js of a legacy extension +const path = require("path"); + +global.$injector.requireCommand( + "hello|world", + path.join(__dirname, "commands", "hello-world"), +); +``` + +This path remains supported, but it is tracked for eventual deprecation. Run any +command with `--log trace` to see which installed extensions still rely on it, +or set `NS_DEPRECATIONS=warn` to have those reports printed as warnings. + +## Writing a command module + +A command module must register itself when it is loaded, by calling +`$injector.registerCommand` at the top level with the same name it is declared +under in the manifest. The CLI resolves the command through the injector right +after loading the module, so registration has to happen as a side effect of the +`require`. + +```js +// dist/commands/hello-world.js +class HelloWorldCommand { + constructor($logger) { + this.$logger = $logger; + this.allowedParameters = []; + } + + async execute(args) { + this.$logger.info(`Hello, ${args[0] || "world"}!`); + } +} + +global.$injector.registerCommand("hello|world", HelloWorldCommand); +``` + +Constructor parameters are injected by name: a parameter called `$logger` +receives the CLI's logger, `$fs` its file system service, and so on. A command +class must expose `allowedParameters` and an `execute(args)` method. + +## Command names + +Command names use `|` to express hierarchy, so `"hello|world"` is invoked as +`ns hello world`. Prefixing the last segment with `*` marks a default +subcommand: `"hello|*default"` runs both for `ns hello default` and for a bare +`ns hello`. + +When an extension contributes several commands under the same parent, declare +the default command before its siblings — the CLI creates the parent dispatcher +from the first entry it sees, and a default command registered after that parent +already exists is rejected. + +## Suggesting an extension for an unknown command + +When a user types a command the CLI does not know, it searches npm for packages +carrying the `nativescript:extension` keyword, reads the `nativescript.commands` +key of each candidate's published `package.json`, and matches it against the +words the user typed — longest match first, so `ns valid command with args` +matches a declared `valid|command|with` before `valid|command`. A declared +default command also matches its short form: an extension declaring +`hello|*default` is suggested for a bare `ns hello`. + +Both manifest shapes participate in this matching. If a match is found, the CLI +tells the user which extension provides the command and how to install it: + +``` +The command hello world is registered in extension nativescript-hello. +You can install it by executing 'ns extension install nativescript-hello' +``` + +## Documentation + +Point the `docs` key of the `nativescript` key at a directory of `.md` files to +have the CLI's help system pick up the help for your commands. + +```json +{ + "nativescript": { + "docs": "./docs", + "commands": { + "hello|world": "./dist/commands/hello-world.js" + } + } +} +``` diff --git a/lib/common/definitions/extensibility.d.ts b/lib/common/definitions/extensibility.d.ts index fbfd3be6d5..dceac2005c 100644 --- a/lib/common/definitions/extensibility.d.ts +++ b/lib/common/definitions/extensibility.d.ts @@ -30,6 +30,13 @@ interface IExtensionData extends IExtensionName { * Full path to the directory of the installed extension. */ pathToExtension: string; + + /** + * Names of the commands the extension contributes, as declared in the commands key of the nativescript key of its package.json. + * The key may be a map of command name to the module implementing it, in which case these are its keys, or the legacy array of command names, in which case these are its entries. + * The property is not set when the extension declares no commands. + */ + commands?: string[]; } /** diff --git a/lib/services/extensibility-service.ts b/lib/services/extensibility-service.ts index 4129c2b9d6..44bb1f2378 100644 --- a/lib/services/extensibility-service.ts +++ b/lib/services/extensibility-service.ts @@ -19,9 +19,40 @@ import { } from "../common/definitions/extensibility"; import { injector } from "../common/yok"; +function isNonEmptyString(value: any): boolean { + return typeof value === "string" && value.trim().length > 0; +} + +function isCommandsMap(commands: any): boolean { + return !!commands && typeof commands === "object" && !Array.isArray(commands); +} + +/** + * Reads the names of the commands an extension contributes out of either shape + * of `nativescript.commands` - the legacy array of names, or the map of name to + * module path. + */ +function getDeclaredCommandNames( + commands: any, + opts?: { copy: boolean }, +): string[] { + if (Array.isArray(commands)) { + return opts && opts.copy ? commands.slice() : commands; + } + + if (isCommandsMap(commands)) { + return _.keys(commands); + } + + return null; +} + export class ExtensibilityService implements IExtensibilityService { private customPathToExtensions: string = null; + /** Command name -> name of the extension whose manifest claimed it first. */ + private manifestCommandOwners: IStringDictionary = {}; + private get pathToPackageJson(): string { return path.join(this.pathToExtensions, constants.PACKAGE_JSON_FILE_NAME); } @@ -132,12 +163,23 @@ export class ExtensibilityService implements IExtensibilityService { packageJsonData.nativescript && packageJsonData.nativescript.docs && path.join(pathToExtension, packageJsonData.nativescript.docs); - return { + const result: IExtensionData = { extensionName: packageJsonData.name, version: packageJsonData.version, docs, pathToExtension, }; + + const commands = getDeclaredCommandNames( + packageJsonData && + packageJsonData.nativescript && + packageJsonData.nativescript.commands, + ); + if (commands) { + result.commands = commands; + } + + return result; } public async loadExtension(extensionName: string): Promise { @@ -145,12 +187,23 @@ export class ExtensibilityService implements IExtensibilityService { await this.assertExtensionIsInstalled(extensionName); const pathToExtension = this.getPathToExtension(extensionName); - reportDeprecation({ - api: "extensions.require-time-registration", - detail: extensionName, - logger: this.$logger, - }); - this.$requireService.require(pathToExtension); + const commandsMap = this.getDeclaredCommandsMap(extensionName); + + if (commandsMap) { + this.registerDeclaredCommands( + extensionName, + pathToExtension, + commandsMap, + ); + } else { + reportDeprecation({ + api: "extensions.require-time-registration", + detail: extensionName, + logger: this.$logger, + }); + this.$requireService.require(pathToExtension); + } + return this.getInstalledExtensionData(extensionName); } catch (error) { this.$logger.warn( @@ -200,10 +253,13 @@ export class ExtensibilityService implements IExtensibilityService { await this.$packageManager.getRegistryPackageData(extensionName); const latestPackageData = registryData.versions[registryData["dist-tags"].latest]; - const commands: string[] = + const commands = getDeclaredCommandNames( latestPackageData && - latestPackageData.nativescript && - latestPackageData.nativescript.commands; + latestPackageData.nativescript && + latestPackageData.nativescript.commands, + // The |* synthesis below pushes into this array. + { copy: true }, + ); if (commands && commands.length) { // For each default command we need to add its short syntax in the array of commands. // For example in case there's a default command called devices list, the commands array will contain devices|*list. @@ -249,6 +305,74 @@ export class ExtensibilityService implements IExtensibilityService { return null; } + /** + * Returns the `nativescript.commands` value of an extension only when it is a + * map of command name to module path. Any other shape (the legacy array of + * command names, a missing key, an unreadable package.json) yields null and + * keeps the extension on the eager require path. + */ + private getDeclaredCommandsMap(extensionName: string): IStringDictionary { + let commands: any; + + try { + const packageJsonData = this.getExtensionPackageJsonData(extensionName); + commands = + packageJsonData && + packageJsonData.nativescript && + packageJsonData.nativescript.commands; + } catch (err) { + this.$logger.trace( + `Unable to read the package.json of extension ${extensionName}. Error is: ${err}`, + ); + return null; + } + + return isCommandsMap(commands) ? commands : null; + } + + /** + * Registers each declared command as a deferred require of its own module, so + * nothing from the extension is loaded until one of its commands is executed. + * Each module is expected to register itself on load, by calling + * `$injector.registerCommand(, )` at the top level. + */ + private registerDeclaredCommands( + extensionName: string, + pathToExtension: string, + commands: IStringDictionary, + ): void { + for (const commandName of _.keys(commands)) { + const modulePath = commands[commandName]; + + if (!isNonEmptyString(commandName) || !isNonEmptyString(modulePath)) { + this.$logger.warn( + `Extension ${extensionName} declares an invalid command in its nativescript.commands: '${commandName}': ${JSON.stringify( + modulePath, + )}. Both the command name and the path to its module must be non-empty strings. Skipping this command.`, + ); + continue; + } + + try { + injector.requireCommand( + commandName, + path.join(pathToExtension, modulePath), + ); + } catch (err) { + const owner = this.manifestCommandOwners[commandName]; + const ownerInfo = owner + ? ` It is already registered by extension ${owner}.` + : ""; + this.$logger.warn( + `Extension ${extensionName} is unable to register command ${commandName}.${ownerInfo} Error: ${err.message}`, + ); + continue; + } + + this.manifestCommandOwners[commandName] = extensionName; + } + } + private getPathToExtension(extensionName: string): string { return path.join( this.pathToExtensions, diff --git a/test/extension-manifests.ts b/test/extension-manifests.ts new file mode 100644 index 0000000000..f15b76a40f --- /dev/null +++ b/test/extension-manifests.ts @@ -0,0 +1,460 @@ +import { assert } from "chai"; +import * as fs from "fs"; +import * as os from "os"; +import * as path from "path"; +import { ExtensibilityService } from "../lib/services/extensibility-service"; +import { Yok } from "../lib/common/yok"; +import { LoggerStub } from "./stubs"; +import { clearReportedDeprecations } from "../lib/common/deprecation"; +import { CommandsDelimiters } from "../lib/common/constants"; +import { ICliGlobal } from "../lib/common/definitions/cli-global"; +import { IInjector } from "../lib/common/definitions/yok"; +import { + IExtensibilityService, + IExtensionData, +} from "../lib/common/definitions/extensibility"; +import { IStringDictionary } from "../lib/common/declarations"; + +// The service registers commands on the global injector imported by +// lib/common/yok, so every assertion about registered commands goes through it. +// That injector is shared by the whole file, so command names must be unique per +// test - a name reused across tests would collide like a real conflict does. +const cliGlobal = (global); + +interface ITestCapture { + loadedModules: string[]; + executed: any[]; +} + +const DEPRECATION_API = "extensions.require-time-registration"; + +describe("extension manifests", () => { + let profileDir: string; + let requiredPaths: string[]; + let capture: ITestCapture; + + beforeEach(() => { + profileDir = fs.mkdtempSync(path.join(os.tmpdir(), "ns-ext-manifest-")); + requiredPaths = []; + capture = (global).__nsmCapture = { + loadedModules: [], + executed: [], + }; + fs.mkdirSync(path.join(profileDir, "extensions", "node_modules"), { + recursive: true, + }); + writeExtensionsPackageJson({}); + clearReportedDeprecations(); + }); + + afterEach(() => { + fs.rmSync(profileDir, { recursive: true, force: true }); + delete (global).__nsmCapture; + }); + + const writeExtensionsPackageJson = ( + dependencies: IStringDictionary, + ): void => { + fs.writeFileSync( + path.join(profileDir, "extensions", "package.json"), + JSON.stringify({ + name: "nativescript-extensibility", + version: "1.0.0", + dependencies, + }), + ); + }; + + /** + * Lays out a real extension package: package.json with the given nativescript + * key plus the given files, and an entry in the extensions dir dependencies. + */ + const writeExtension = ( + extensionName: string, + nativescript: any, + files: IStringDictionary, + ): string => { + const pathToExtension = path.join( + profileDir, + "extensions", + "node_modules", + extensionName, + ); + fs.mkdirSync(pathToExtension, { recursive: true }); + fs.writeFileSync( + path.join(pathToExtension, "package.json"), + JSON.stringify({ + name: extensionName, + version: "1.0.0", + main: "main.js", + nativescript, + }), + ); + + for (const relativePath of Object.keys(files || {})) { + const pathToFile = path.join(pathToExtension, relativePath); + fs.mkdirSync(path.dirname(pathToFile), { recursive: true }); + fs.writeFileSync(pathToFile, files[relativePath]); + } + + const pathToExtensionsPackageJson = path.join( + profileDir, + "extensions", + "package.json", + ); + const packageJsonData = JSON.parse( + fs.readFileSync(pathToExtensionsPackageJson).toString(), + ); + packageJsonData.dependencies[extensionName] = "1.0.0"; + fs.writeFileSync( + pathToExtensionsPackageJson, + JSON.stringify(packageJsonData), + ); + + return pathToExtension; + }; + + const mainModule = (marker: string): string => + `global.__nsmCapture.loadedModules.push(${JSON.stringify(marker)});`; + + const commandModule = (commandName: string, marker: string): string => + `class TestCommand { + constructor() { + this.allowedParameters = []; + } + async execute(args) { + global.__nsmCapture.executed.push({ marker: ${JSON.stringify( + marker, + )}, args: args }); + } + } + global.__nsmCapture.loadedModules.push(${JSON.stringify(marker)}); + global.$injector.registerCommand(${JSON.stringify(commandName)}, TestCommand);`; + + const getTestInjector = (): IInjector => { + const testInjector = new Yok(); + testInjector.register("fs", { + exists: (pathToCheck: string): boolean => fs.existsSync(pathToCheck), + readJson: (pathToFile: string): any => + JSON.parse(fs.readFileSync(pathToFile).toString()), + readText: (pathToFile: string): string => + fs.readFileSync(pathToFile).toString(), + readDirectory: (dir: string): string[] => fs.readdirSync(dir), + createDirectory: (dir: string): void => { + fs.mkdirSync(dir, { recursive: true }); + }, + writeJson: (pathToFile: string, content: any): void => + fs.writeFileSync(pathToFile, JSON.stringify(content)), + }); + testInjector.register("logger", LoggerStub); + testInjector.register("packageManager", { + install: async (): Promise => { + throw new Error("Extensions are expected to be installed already."); + }, + uninstall: async (): Promise => undefined, + searchNpms: async (): Promise => ({ results: [] }), + getRegistryPackageData: async (): Promise => ({}), + }); + testInjector.register("settingsService", { + getProfileDir: (): string => profileDir, + }); + testInjector.register("requireService", { + require: (module: string): any => { + requiredPaths.push(module); + return require(module); + }, + }); + + return testInjector; + }; + + const resolveService = (testInjector: IInjector): IExtensibilityService => + testInjector.resolve(ExtensibilityService); + + const getLogger = (testInjector: IInjector): LoggerStub => + testInjector.resolve("logger"); + + describe("commands declared as a map", () => { + it("registers each command lazily and never loads the extension main", async () => { + const extensionName = "nsm-lazy-ext"; + writeExtension( + extensionName, + { + commands: { + "nsmlazy|run": "./dist/commands/run.js", + "nsmlazy|clean": "./dist/commands/clean.js", + }, + }, + { + "main.js": mainModule("lazy-main"), + "dist/commands/run.js": commandModule("nsmlazy|run", "lazy-run"), + "dist/commands/clean.js": commandModule( + "nsmlazy|clean", + "lazy-clean", + ), + }, + ); + + const testInjector = getTestInjector(); + const extensibilityService = resolveService(testInjector); + const extensionData = + await extensibilityService.loadExtension(extensionName); + + assert.deepStrictEqual(capture.loadedModules, []); + assert.deepStrictEqual( + requiredPaths, + [], + "The extension main must not be required when its commands are declared as a map.", + ); + assert.notInclude(getLogger(testInjector).traceOutput, DEPRECATION_API); + assert.deepStrictEqual(extensionData.commands, [ + "nsmlazy|run", + "nsmlazy|clean", + ]); + + const command = cliGlobal.$injector.resolveCommand("nsmlazy|run"); + assert.isOk(command); + assert.deepStrictEqual(capture.loadedModules, ["lazy-run"]); + + await command.execute(["arg"]); + assert.deepStrictEqual(capture.executed, [ + { marker: "lazy-run", args: ["arg"] }, + ]); + + // The other command's module is still not loaded, and neither is main. + assert.deepStrictEqual(capture.loadedModules, ["lazy-run"]); + assert.deepStrictEqual(requiredPaths, []); + }); + + it("warns about and skips malformed entries, keeping the valid ones", async () => { + const extensionName = "nsm-malformed-ext"; + writeExtension( + extensionName, + { + commands: { + "nsmbad|good": "./good.js", + "nsmbad|number": 42, + "nsmbad|empty": " ", + "": "./unnamed.js", + }, + }, + { + "main.js": mainModule("malformed-main"), + "good.js": commandModule("nsmbad|good", "malformed-good"), + }, + ); + + const testInjector = getTestInjector(); + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + const warnOutput = getLogger(testInjector).warnOutput; + assert.include(warnOutput, "nsmbad|number"); + assert.include(warnOutput, "nsmbad|empty"); + assert.include(warnOutput, extensionName); + + assert.isOk(cliGlobal.$injector.resolveCommand("nsmbad|good")); + assert.isNull(cliGlobal.$injector.resolveCommand("nsmbad|number")); + assert.isNull(cliGlobal.$injector.resolveCommand("nsmbad|empty")); + assert.deepStrictEqual(capture.loadedModules, ["malformed-good"]); + }); + + it("warns instead of failing when two extensions claim the same command", async () => { + const firstExtension = "nsm-first-ext"; + const secondExtension = "nsm-second-ext"; + writeExtension( + firstExtension, + { commands: { "nsmconflict|run": "./run.js" } }, + { + "main.js": mainModule("first-main"), + "run.js": commandModule("nsmconflict|run", "first-run"), + }, + ); + writeExtension( + secondExtension, + { commands: { "nsmconflict|run": "./run.js" } }, + { + "main.js": mainModule("second-main"), + "run.js": commandModule("nsmconflict|run", "second-run"), + }, + ); + + const testInjector = getTestInjector(); + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(firstExtension); + await extensibilityService.loadExtension(secondExtension); + + const warnOutput = getLogger(testInjector).warnOutput; + assert.include(warnOutput, "nsmconflict|run"); + assert.include(warnOutput, firstExtension); + assert.include(warnOutput, secondExtension); + + const command = cliGlobal.$injector.resolveCommand("nsmconflict|run"); + await command.execute([]); + assert.deepStrictEqual(capture.executed, [ + { marker: "first-run", args: [] }, + ]); + }); + }); + + describe("commands declared as an array", () => { + it("requires the extension main eagerly and reports the deprecated registration", async () => { + const extensionName = "nsm-eager-ext"; + const pathToExtension = writeExtension( + extensionName, + { commands: ["nsmeager|run"] }, + { + "main.js": `${mainModule("eager-main")} + ${commandModule("nsmeager|run", "eager-run")}`, + }, + ); + + const testInjector = getTestInjector(); + const extensibilityService = resolveService(testInjector); + const extensionData = + await extensibilityService.loadExtension(extensionName); + + assert.deepStrictEqual(requiredPaths, [pathToExtension]); + assert.deepStrictEqual(capture.loadedModules, [ + "eager-main", + "eager-run", + ]); + + const traceOutput = getLogger(testInjector).traceOutput; + assert.include(traceOutput, DEPRECATION_API); + assert.include(traceOutput, extensionName); + + assert.deepStrictEqual(extensionData.commands, ["nsmeager|run"]); + assert.isOk(cliGlobal.$injector.resolveCommand("nsmeager|run")); + }); + + it("keeps the eager path when the extension declares no commands", async () => { + const extensionName = "nsm-no-commands-ext"; + const pathToExtension = writeExtension( + extensionName, + { docs: "./docs" }, + { "main.js": mainModule("no-commands-main") }, + ); + + const testInjector = getTestInjector(); + const extensibilityService = resolveService(testInjector); + const extensionData = + await extensibilityService.loadExtension(extensionName); + + assert.deepStrictEqual(requiredPaths, [pathToExtension]); + assert.include(getLogger(testInjector).traceOutput, DEPRECATION_API); + assert.isUndefined(extensionData.commands); + }); + }); + + describe("getInstalledExtensionsData", () => { + it("reports the declared command names for both manifest shapes", () => { + writeExtension( + "nsm-data-map-ext", + { commands: { "nsmdata|one": "./one.js", "nsmdata|two": "./two.js" } }, + {}, + ); + writeExtension("nsm-data-array-ext", { commands: ["nsmdata|three"] }, {}); + writeExtension("nsm-data-plain-ext", {}, {}); + + const testInjector = getTestInjector(); + const extensibilityService = resolveService(testInjector); + const extensionsData = extensibilityService.getInstalledExtensionsData(); + const dataByName: { [name: string]: IExtensionData } = {}; + for (const extensionData of extensionsData) { + dataByName[extensionData.extensionName] = extensionData; + } + + assert.deepStrictEqual(dataByName["nsm-data-map-ext"].commands, [ + "nsmdata|one", + "nsmdata|two", + ]); + assert.deepStrictEqual(dataByName["nsm-data-array-ext"].commands, [ + "nsmdata|three", + ]); + assert.isUndefined(dataByName["nsm-data-plain-ext"].commands); + }); + }); + + describe("getExtensionNameWhereCommandIsRegistered", () => { + const getExtensionCommandInfo = async ( + registryCommands: any, + inputStrings: string[], + ): Promise => { + const extensionName = "nsm-registry-ext"; + const testInjector = getTestInjector(); + const packageManager = testInjector.resolve("packageManager"); + packageManager.searchNpms = async (keyword: string): Promise => { + assert.equal(keyword, "nativescript:extension"); + return { results: [{ package: { name: extensionName } }] }; + }; + packageManager.getRegistryPackageData = async (): Promise => ({ + ["dist-tags"]: { latest: "1.0.0" }, + versions: { + "1.0.0": { nativescript: { commands: registryCommands } }, + }, + }); + + const extensibilityService = resolveService(testInjector); + return extensibilityService.getExtensionNameWhereCommandIsRegistered({ + inputStrings, + commandDelimiter: CommandsDelimiters.HierarchicalCommand, + defaultCommandDelimiter: CommandsDelimiters.DefaultHierarchicalCommand, + }); + }; + + it("suggests an extension whose registry data declares commands as a map", async () => { + const result = await getExtensionCommandInfo( + { "registry|command": "./registry-command.js" }, + ["registry", "command", "and", "args"], + ); + + assert.deepStrictEqual(result, { + extensionName: "nsm-registry-ext", + registeredCommandName: "registry|command", + installationMessage: + "The command registry command is registered in extension nsm-registry-ext. You can install it by executing 'ns extension install nsm-registry-ext'", + }); + }); + + it("synthesizes the short form of a default command declared as a map", async () => { + const result = await getExtensionCommandInfo( + { + "registry|*default": "./registry-default.js", + "registry|other": "./registry-other.js", + }, + ["registry", "and", "args"], + ); + + assert.deepStrictEqual(result, { + extensionName: "nsm-registry-ext", + registeredCommandName: "registry", + installationMessage: + "The command registry is registered in extension nsm-registry-ext. You can install it by executing 'ns extension install nsm-registry-ext'", + }); + }); + + it("still suggests an extension whose registry data declares commands as an array", async () => { + const result = await getExtensionCommandInfo( + ["registry|*default", "registry|other"], + ["registry", "and", "args"], + ); + + assert.deepStrictEqual(result, { + extensionName: "nsm-registry-ext", + registeredCommandName: "registry", + installationMessage: + "The command registry is registered in extension nsm-registry-ext. You can install it by executing 'ns extension install nsm-registry-ext'", + }); + }); + + it("returns null when the declared commands do not match the input", async () => { + const result = await getExtensionCommandInfo( + { "registry|command": "./registry-command.js" }, + ["some", "other", "command"], + ); + + assert.isNull(result); + }); + }); +}); From 29c6872c85e20c96f87d0d008015097e856aa32a Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 22:08:47 -0300 Subject: [PATCH 10/18] feat(extensions): load defineCommand modules from manifest entries A manifest entry may now point at a module that exports a defineCommand definition instead of registering itself on load: the deferred loader adapts and registers the export under the manifest key. The override also lands on a parent record the entry just created, because dispatch resolves the hierarchical parent before any child module has loaded and the dispatcher only comes into existence once a child registers. Also cross-links the authoring guides from dependency-injection.md. --- defining-commands.md | 7 +-- dependency-injection.md | 7 +++ extensions.md | 20 +++++++- lib/services/extensibility-service.ts | 48 +++++++++++++++--- test/extension-manifests.ts | 70 +++++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 10 deletions(-) diff --git a/defining-commands.md b/defining-commands.md index b6f589cf9a..ef7fc024c1 100644 --- a/defining-commands.md +++ b/defining-commands.md @@ -322,9 +322,10 @@ side-effect-free contracts entry point deliberately does not pull it in. `defineCommand`, the option helpers and all the types are exported from both `nativescript/contracts` and `lib/common/define-command`. -Declaring commands from an extension manifest, so that an extension does not -have to call a registration function at load time, is being added separately. -Until then, extensions register definitions the same way the CLI does. +Extensions do not need `registerCommandDefinition` at all: a +`nativescript.commands` manifest entry may point straight at a module that +exports a definition, and the CLI adapts and registers it lazily under the +manifest key (see [extensions.md](extensions.md)). Relationship to `ICommand` -------------------------- diff --git a/dependency-injection.md b/dependency-injection.md index b8386c1f9b..98fcd7e08b 100644 --- a/dependency-injection.md +++ b/dependency-injection.md @@ -265,6 +265,13 @@ The first tranche, growing as services migrate: | `DoctorService` | `doctorService` | | `ProjectNameService` | `projectNameService` | +Related guides +-------------- + +- [defining-commands.md](defining-commands.md) — declarative, typed commands via `defineCommand`. +- [extensions.md](extensions.md) — extension authoring, including the `nativescript.commands` manifest. +- [extending-cli.md](extending-cli.md) — hooks, including the typed `defineHook` API. + Legacy → new quick reference ---------------------------- diff --git a/extensions.md b/extensions.md index f92c00f1dc..787d94ac87 100644 --- a/extensions.md +++ b/extensions.md @@ -106,7 +106,25 @@ or set `NS_DEPRECATIONS=warn` to have those reports printed as warnings. ## Writing a command module -A command module must register itself when it is loaded, by calling +The recommended shape is a module exporting a `defineCommand` definition (see +[defining-commands.md](defining-commands.md)) — the CLI adapts and registers it +under the manifest key when the command is first executed, and the module needs +no registration side effects at all: + +```js +// dist/commands/hello-world.js +const { defineCommand, inject } = require("nativescript/contracts"); + +module.exports = defineCommand({ + name: "hello|world", + arguments: "any", + async run(ctx) { + inject("logger").info(`Hello, ${ctx.args[0] || "world"}!`); + }, +}); +``` + +Alternatively, a module may register itself when it is loaded, by calling `$injector.registerCommand` at the top level with the same name it is declared under in the manifest. The CLI resolves the command through the injector right after loading the module, so registration has to happen as a side effect of the diff --git a/lib/services/extensibility-service.ts b/lib/services/extensibility-service.ts index 44bb1f2378..48ff78141e 100644 --- a/lib/services/extensibility-service.ts +++ b/lib/services/extensibility-service.ts @@ -18,6 +18,9 @@ import { IGetExtensionCommandInfoParams, } from "../common/definitions/extensibility"; import { injector } from "../common/yok"; +import { CommandsDelimiters } from "../common/constants"; +import { isCommandDefinition } from "../common/define-command"; +import { createCommandFromDefinition } from "../common/services/command-definition-adapter"; function isNonEmptyString(value: any): boolean { return typeof value === "string" && value.trim().length > 0; @@ -333,8 +336,10 @@ export class ExtensibilityService implements IExtensibilityService { /** * Registers each declared command as a deferred require of its own module, so * nothing from the extension is loaded until one of its commands is executed. - * Each module is expected to register itself on load, by calling - * `$injector.registerCommand(, )` at the top level. + * A module may either register itself on load (a legacy-style + * `$injector.registerCommand(, )` at the top level) or export a + * `defineCommand` definition, which the deferred loader adapts and registers + * under the manifest key. */ private registerDeclaredCommands( extensionName: string, @@ -353,11 +358,16 @@ export class ExtensibilityService implements IExtensibilityService { continue; } + const absoluteModulePath = path.join(pathToExtension, modulePath); + const parentName = commandName.split( + CommandsDelimiters.HierarchicalCommand, + )[0]; + const container = (injector).di; + const parentWasAbsent = + parentName !== commandName && !container.has(`commands.${parentName}`); + try { - injector.requireCommand( - commandName, - path.join(pathToExtension, modulePath), - ); + injector.requireCommand(commandName, absoluteModulePath); } catch (err) { const owner = this.manifestCommandOwners[commandName]; const ownerInfo = owner @@ -369,6 +379,32 @@ export class ExtensibilityService implements IExtensibilityService { continue; } + // requireCommand's own loader only require()s the module for its side + // effects, which covers self-registering modules but not definition + // exports. The override must also land on a parent record this entry + // just created: dispatch resolves the parent BEFORE any child module + // has loaded, and the parent dispatcher only comes into existence once + // a child's registerCommand runs. + const loader = () => { + const exported = require(absoluteModulePath); + const candidate = (exported && exported.default) ?? exported; + if (isCommandDefinition(candidate)) { + injector.registerCommand(commandName, () => + createCommandFromDefinition(candidate), + ); + } + }; + container.register({ + provide: `commands.${commandName}`, + useLazyRequire: loader, + }); + if (parentWasAbsent) { + container.register({ + provide: `commands.${parentName}`, + useLazyRequire: loader, + }); + } + this.manifestCommandOwners[commandName] = extensionName; } } diff --git a/test/extension-manifests.ts b/test/extension-manifests.ts index f15b76a40f..770639702f 100644 --- a/test/extension-manifests.ts +++ b/test/extension-manifests.ts @@ -457,4 +457,74 @@ describe("extension manifests", () => { assert.isNull(result); }); }); + + describe("commands declared as defineCommand modules", () => { + const contractsPath = require.resolve("../lib/contracts"); + + const definitionModule = (commandName: string, marker: string): string => + `const { defineCommand } = require(${JSON.stringify(contractsPath)}); + global.__nsmCapture.loadedModules.push(${JSON.stringify(marker)}); + module.exports = defineCommand({ + name: ${JSON.stringify(commandName)}, + arguments: "any", + async run(ctx) { + global.__nsmCapture.executed.push({ marker: ${JSON.stringify( + marker, + )}, args: ctx.args }); + }, + });`; + + it("adapts and registers a pure definition module lazily", async () => { + const extensionName = "nsm-def-ext"; + writeExtension( + extensionName, + { commands: { "nsmdef|hello": "./dist/hello.js" } }, + { + "main.js": mainModule("def-main"), + "dist/hello.js": definitionModule("nsmdef|hello", "def-hello"), + }, + ); + + const testInjector = getTestInjector(); + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + assert.deepEqual(capture.loadedModules, []); + + const command = cliGlobal.$injector.resolveCommand("nsmdef|hello"); + assert.isOk(command); + assert.deepEqual(capture.loadedModules, ["def-hello"]); + + await command.execute(["fast"]); + assert.deepEqual(capture.executed, [ + { marker: "def-hello", args: ["fast"] }, + ]); + }); + + it("resolves the hierarchical parent dispatcher before any child module has loaded", async () => { + const extensionName = "nsm-defp-ext"; + writeExtension( + extensionName, + { commands: { "nsmdefp|go": "./dist/go.js" } }, + { + "main.js": mainModule("defp-main"), + "dist/go.js": definitionModule("nsmdefp|go", "defp-go"), + }, + ); + + const testInjector = getTestInjector(); + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + // Dispatch hits the parent first; its record must load the child module + // so the dispatcher the child's registration synthesizes exists. + const parent = cliGlobal.$injector.resolveCommand("nsmdefp"); + assert.isOk(parent); + assert.isTrue(parent.isHierarchicalCommand); + assert.include(capture.loadedModules, "defp-go"); + + const child = cliGlobal.$injector.resolveCommand("nsmdefp|go"); + assert.isOk(child); + }); + }); }); From 9439a5d0b0cb4e3e3cc4d8e7f872b9131e936c3a Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 22:41:00 -0300 Subject: [PATCH 11/18] refactor(extensions): inject the injector; keep globals to the legacy seam The service takes $injector as a constructor dependency instead of the module-level import, so manifest registration and the definition-aware loaders target the instance that resolved it. Tests assert on their own per-test injector; the process-wide injector is swapped only because legacy-shape fixture modules register through the published global surface at load, and that seam is labeled as such. extensions.md no longer teaches the global-injector patterns: the legacy array path and self-registering modules are described under their deprecation framing without runnable samples. --- extensions.md | 52 +++++++-------------------- lib/services/extensibility-service.ts | 8 +++-- test/extension-manifests.ts | 48 ++++++++++++------------- 3 files changed, 41 insertions(+), 67 deletions(-) diff --git a/extensions.md b/extensions.md index 787d94ac87..b8feb9a035 100644 --- a/extensions.md +++ b/extensions.md @@ -88,21 +88,14 @@ The array is a discovery aid only — it lists the names the CLI may suggest you extension for, but it says nothing about where the implementations live. An extension declaring commands this way (or declaring no commands at all) is loaded the old way: the CLI `require()`s the package's main entry on **every** -invocation and expects the module's top-level code to register everything. +invocation and expects the module's top-level code to register everything +through the injector global. -```js -// index.js of a legacy extension -const path = require("path"); - -global.$injector.requireCommand( - "hello|world", - path.join(__dirname, "commands", "hello-world"), -); -``` - -This path remains supported, but it is tracked for eventual deprecation. Run any -command with `--log trace` to see which installed extensions still rely on it, -or set `NS_DEPRECATIONS=warn` to have those reports printed as warnings. +This path remains supported for published extensions, but it is tracked for +eventual deprecation and new extensions should not use it — declare the map +instead. Run any command with `--log trace` to see which installed extensions +still rely on it, or set `NS_DEPRECATIONS=warn` to have those reports printed +as warnings. ## Writing a command module @@ -124,31 +117,12 @@ module.exports = defineCommand({ }); ``` -Alternatively, a module may register itself when it is loaded, by calling -`$injector.registerCommand` at the top level with the same name it is declared -under in the manifest. The CLI resolves the command through the injector right -after loading the module, so registration has to happen as a side effect of the -`require`. - -```js -// dist/commands/hello-world.js -class HelloWorldCommand { - constructor($logger) { - this.$logger = $logger; - this.allowedParameters = []; - } - - async execute(args) { - this.$logger.info(`Hello, ${args[0] || "world"}!`); - } -} - -global.$injector.registerCommand("hello|world", HelloWorldCommand); -``` - -Constructor parameters are injected by name: a parameter called `$logger` -receives the CLI's logger, `$fs` its file system service, and so on. A command -class must expose `allowedParameters` and an `execute(args)` method. +Legacy modules — command classes that register themselves at load time through +the injector global, with parameter-name constructor injection — keep working +when a manifest entry points at them, so existing extensions can adopt the map +without rewriting their commands. Both of those mechanisms are deprecated +(see [dependency-injection.md](dependency-injection.md)); write new modules as +definitions. ## Command names diff --git a/lib/services/extensibility-service.ts b/lib/services/extensibility-service.ts index 48ff78141e..37ec05ef07 100644 --- a/lib/services/extensibility-service.ts +++ b/lib/services/extensibility-service.ts @@ -18,6 +18,7 @@ import { IGetExtensionCommandInfoParams, } from "../common/definitions/extensibility"; import { injector } from "../common/yok"; +import { IInjector } from "../common/definitions/yok"; import { CommandsDelimiters } from "../common/constants"; import { isCommandDefinition } from "../common/define-command"; import { createCommandFromDefinition } from "../common/services/command-definition-adapter"; @@ -77,6 +78,7 @@ export class ExtensibilityService implements IExtensibilityService { private $packageManager: INodePackageManager, private $settingsService: ISettingsService, private $requireService: IRequireService, + private $injector: IInjector, ) {} public async installExtension( @@ -362,12 +364,12 @@ export class ExtensibilityService implements IExtensibilityService { const parentName = commandName.split( CommandsDelimiters.HierarchicalCommand, )[0]; - const container = (injector).di; + const container = (this.$injector).di; const parentWasAbsent = parentName !== commandName && !container.has(`commands.${parentName}`); try { - injector.requireCommand(commandName, absoluteModulePath); + this.$injector.requireCommand(commandName, absoluteModulePath); } catch (err) { const owner = this.manifestCommandOwners[commandName]; const ownerInfo = owner @@ -389,7 +391,7 @@ export class ExtensibilityService implements IExtensibilityService { const exported = require(absoluteModulePath); const candidate = (exported && exported.default) ?? exported; if (isCommandDefinition(candidate)) { - injector.registerCommand(commandName, () => + this.$injector.registerCommand(commandName, () => createCommandFromDefinition(candidate), ); } diff --git a/test/extension-manifests.ts b/test/extension-manifests.ts index 770639702f..4bfd387d44 100644 --- a/test/extension-manifests.ts +++ b/test/extension-manifests.ts @@ -3,11 +3,10 @@ import * as fs from "fs"; import * as os from "os"; import * as path from "path"; import { ExtensibilityService } from "../lib/services/extensibility-service"; -import { Yok } from "../lib/common/yok"; +import { Yok, getInjector, setGlobalInjector } from "../lib/common/yok"; import { LoggerStub } from "./stubs"; import { clearReportedDeprecations } from "../lib/common/deprecation"; import { CommandsDelimiters } from "../lib/common/constants"; -import { ICliGlobal } from "../lib/common/definitions/cli-global"; import { IInjector } from "../lib/common/definitions/yok"; import { IExtensibilityService, @@ -15,11 +14,13 @@ import { } from "../lib/common/definitions/extensibility"; import { IStringDictionary } from "../lib/common/declarations"; -// The service registers commands on the global injector imported by -// lib/common/yok, so every assertion about registered commands goes through it. -// That injector is shared by the whole file, so command names must be unique per -// test - a name reused across tests would collide like a real conflict does. -const cliGlobal = (global); +// Every assertion about registered commands goes through the per-test +// injector: the service takes $injector as a constructor dependency. The +// process-wide injector is pointed at that same instance for each test's +// duration ONLY because legacy-shape fixture modules register through the +// published global surface when they load - that swap is the legacy-compat +// seam, not the assertion path. Command names stay unique per test since the +// module require cache outlives a test. interface ITestCapture { loadedModules: string[]; @@ -32,9 +33,14 @@ describe("extension manifests", () => { let profileDir: string; let requiredPaths: string[]; let capture: ITestCapture; + let testInjector: IInjector; + let previousProcessInjector: IInjector; beforeEach(() => { profileDir = fs.mkdtempSync(path.join(os.tmpdir(), "ns-ext-manifest-")); + testInjector = getTestInjector(); + previousProcessInjector = getInjector(); + setGlobalInjector(testInjector); requiredPaths = []; capture = (global).__nsmCapture = { loadedModules: [], @@ -48,6 +54,7 @@ describe("extension manifests", () => { }); afterEach(() => { + setGlobalInjector(previousProcessInjector); fs.rmSync(profileDir, { recursive: true, force: true }); delete (global).__nsmCapture; }); @@ -195,7 +202,6 @@ describe("extension manifests", () => { }, ); - const testInjector = getTestInjector(); const extensibilityService = resolveService(testInjector); const extensionData = await extensibilityService.loadExtension(extensionName); @@ -212,7 +218,7 @@ describe("extension manifests", () => { "nsmlazy|clean", ]); - const command = cliGlobal.$injector.resolveCommand("nsmlazy|run"); + const command = testInjector.resolveCommand("nsmlazy|run"); assert.isOk(command); assert.deepStrictEqual(capture.loadedModules, ["lazy-run"]); @@ -244,7 +250,6 @@ describe("extension manifests", () => { }, ); - const testInjector = getTestInjector(); const extensibilityService = resolveService(testInjector); await extensibilityService.loadExtension(extensionName); @@ -253,9 +258,9 @@ describe("extension manifests", () => { assert.include(warnOutput, "nsmbad|empty"); assert.include(warnOutput, extensionName); - assert.isOk(cliGlobal.$injector.resolveCommand("nsmbad|good")); - assert.isNull(cliGlobal.$injector.resolveCommand("nsmbad|number")); - assert.isNull(cliGlobal.$injector.resolveCommand("nsmbad|empty")); + assert.isOk(testInjector.resolveCommand("nsmbad|good")); + assert.isNull(testInjector.resolveCommand("nsmbad|number")); + assert.isNull(testInjector.resolveCommand("nsmbad|empty")); assert.deepStrictEqual(capture.loadedModules, ["malformed-good"]); }); @@ -279,7 +284,6 @@ describe("extension manifests", () => { }, ); - const testInjector = getTestInjector(); const extensibilityService = resolveService(testInjector); await extensibilityService.loadExtension(firstExtension); await extensibilityService.loadExtension(secondExtension); @@ -289,7 +293,7 @@ describe("extension manifests", () => { assert.include(warnOutput, firstExtension); assert.include(warnOutput, secondExtension); - const command = cliGlobal.$injector.resolveCommand("nsmconflict|run"); + const command = testInjector.resolveCommand("nsmconflict|run"); await command.execute([]); assert.deepStrictEqual(capture.executed, [ { marker: "first-run", args: [] }, @@ -309,7 +313,6 @@ describe("extension manifests", () => { }, ); - const testInjector = getTestInjector(); const extensibilityService = resolveService(testInjector); const extensionData = await extensibilityService.loadExtension(extensionName); @@ -325,7 +328,7 @@ describe("extension manifests", () => { assert.include(traceOutput, extensionName); assert.deepStrictEqual(extensionData.commands, ["nsmeager|run"]); - assert.isOk(cliGlobal.$injector.resolveCommand("nsmeager|run")); + assert.isOk(testInjector.resolveCommand("nsmeager|run")); }); it("keeps the eager path when the extension declares no commands", async () => { @@ -336,7 +339,6 @@ describe("extension manifests", () => { { "main.js": mainModule("no-commands-main") }, ); - const testInjector = getTestInjector(); const extensibilityService = resolveService(testInjector); const extensionData = await extensibilityService.loadExtension(extensionName); @@ -357,7 +359,6 @@ describe("extension manifests", () => { writeExtension("nsm-data-array-ext", { commands: ["nsmdata|three"] }, {}); writeExtension("nsm-data-plain-ext", {}, {}); - const testInjector = getTestInjector(); const extensibilityService = resolveService(testInjector); const extensionsData = extensibilityService.getInstalledExtensionsData(); const dataByName: { [name: string]: IExtensionData } = {}; @@ -382,7 +383,6 @@ describe("extension manifests", () => { inputStrings: string[], ): Promise => { const extensionName = "nsm-registry-ext"; - const testInjector = getTestInjector(); const packageManager = testInjector.resolve("packageManager"); packageManager.searchNpms = async (keyword: string): Promise => { assert.equal(keyword, "nativescript:extension"); @@ -485,13 +485,12 @@ describe("extension manifests", () => { }, ); - const testInjector = getTestInjector(); const extensibilityService = resolveService(testInjector); await extensibilityService.loadExtension(extensionName); assert.deepEqual(capture.loadedModules, []); - const command = cliGlobal.$injector.resolveCommand("nsmdef|hello"); + const command = testInjector.resolveCommand("nsmdef|hello"); assert.isOk(command); assert.deepEqual(capture.loadedModules, ["def-hello"]); @@ -512,18 +511,17 @@ describe("extension manifests", () => { }, ); - const testInjector = getTestInjector(); const extensibilityService = resolveService(testInjector); await extensibilityService.loadExtension(extensionName); // Dispatch hits the parent first; its record must load the child module // so the dispatcher the child's registration synthesizes exists. - const parent = cliGlobal.$injector.resolveCommand("nsmdefp"); + const parent = testInjector.resolveCommand("nsmdefp"); assert.isOk(parent); assert.isTrue(parent.isHierarchicalCommand); assert.include(capture.loadedModules, "defp-go"); - const child = cliGlobal.$injector.resolveCommand("nsmdefp|go"); + const child = testInjector.resolveCommand("nsmdefp|go"); assert.isOk(child); }); }); From 09b95cdbf8309cfc0da58488d8f40a2c1ab12ae8 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 22:44:52 -0300 Subject: [PATCH 12/18] refactor(extensions): use the typed di bridge on IInjector --- lib/services/extensibility-service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/services/extensibility-service.ts b/lib/services/extensibility-service.ts index 37ec05ef07..9c2309c0b3 100644 --- a/lib/services/extensibility-service.ts +++ b/lib/services/extensibility-service.ts @@ -364,7 +364,7 @@ export class ExtensibilityService implements IExtensibilityService { const parentName = commandName.split( CommandsDelimiters.HierarchicalCommand, )[0]; - const container = (this.$injector).di; + const container = this.$injector.di; const parentWasAbsent = parentName !== commandName && !container.has(`commands.${parentName}`); From c57a29d55b4c77da9fbd5297bdf28213ed7fa36c Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 23:28:26 -0300 Subject: [PATCH 13/18] refactor(extensions): inject the CommandRegistry face Registry operations go through the narrow subsystem contract; the full facade stays only for container-record operations (has, provider registration). First consumer of the per-face tokens. --- lib/services/extensibility-service.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/services/extensibility-service.ts b/lib/services/extensibility-service.ts index 9c2309c0b3..417eaf3481 100644 --- a/lib/services/extensibility-service.ts +++ b/lib/services/extensibility-service.ts @@ -20,6 +20,8 @@ import { import { injector } from "../common/yok"; import { IInjector } from "../common/definitions/yok"; import { CommandsDelimiters } from "../common/constants"; +import { inject } from "../common/di/inject"; +import { CommandRegistry } from "../common/contracts"; import { isCommandDefinition } from "../common/define-command"; import { createCommandFromDefinition } from "../common/services/command-definition-adapter"; @@ -57,6 +59,8 @@ export class ExtensibilityService implements IExtensibilityService { /** Command name -> name of the extension whose manifest claimed it first. */ private manifestCommandOwners: IStringDictionary = {}; + private commandRegistry = inject(CommandRegistry); + private get pathToPackageJson(): string { return path.join(this.pathToExtensions, constants.PACKAGE_JSON_FILE_NAME); } @@ -364,12 +368,12 @@ export class ExtensibilityService implements IExtensibilityService { const parentName = commandName.split( CommandsDelimiters.HierarchicalCommand, )[0]; - const container = this.$injector.di; const parentWasAbsent = - parentName !== commandName && !container.has(`commands.${parentName}`); + parentName !== commandName && + !this.$injector.has(`commands.${parentName}`); try { - this.$injector.requireCommand(commandName, absoluteModulePath); + this.commandRegistry.requireCommand(commandName, absoluteModulePath); } catch (err) { const owner = this.manifestCommandOwners[commandName]; const ownerInfo = owner @@ -391,17 +395,17 @@ export class ExtensibilityService implements IExtensibilityService { const exported = require(absoluteModulePath); const candidate = (exported && exported.default) ?? exported; if (isCommandDefinition(candidate)) { - this.$injector.registerCommand(commandName, () => + this.commandRegistry.registerCommand(commandName, () => createCommandFromDefinition(candidate), ); } }; - container.register({ + this.$injector.register({ provide: `commands.${commandName}`, useLazyRequire: loader, }); if (parentWasAbsent) { - container.register({ + this.$injector.register({ provide: `commands.${parentName}`, useLazyRequire: loader, }); From 4ca596b58a6f32924b9c2479ae7b66a56df04f7a Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 16:41:39 -0300 Subject: [PATCH 14/18] refactor(di): keep the deferred-loader provider internal to the container A record carrying only a lazy-require loader resolves to an error until the loader registers something onto it, so the form is not one callers should be offered: drop ILazyRequireProvider from the exported Provider union and keep it in an InternalProvider alias the container accepts. Add hasResolver() so the deferred paths can tell a record that a loader has filled in from one it left empty. --- lib/common/di/index.ts | 1 + lib/common/di/injector.ts | 24 +++++++++++++++++++++--- lib/common/di/providers.ts | 10 +++++++--- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/lib/common/di/index.ts b/lib/common/di/index.ts index 1ddf6a1dca..88a5e6f9b6 100644 --- a/lib/common/di/index.ts +++ b/lib/common/di/index.ts @@ -12,6 +12,7 @@ export type { IContractOptions } from "./contract"; export { provide, provideLazy } from "./providers"; export type { Provider, + InternalProvider, ProviderToken, Type, AbstractType, diff --git a/lib/common/di/injector.ts b/lib/common/di/injector.ts index 55a89e2482..12daa16412 100644 --- a/lib/common/di/injector.ts +++ b/lib/common/di/injector.ts @@ -2,7 +2,12 @@ import { annotate } from "../helpers"; import { getContractName } from "./contract"; import { resolveForwardRef } from "./forward-ref"; import { runInInjectionContext } from "./inject"; -import type { Provider, ProviderToken, Type } from "./providers"; +import type { + InternalProvider, + Provider, + ProviderToken, + Type, +} from "./providers"; type TokenKey = string | Function; @@ -131,7 +136,7 @@ export class Injector { } /** Merge-mutate: re-registering a key updates the existing record in place. */ - public register(providers: Provider | Provider[]): void { + public register(providers: InternalProvider | InternalProvider[]): void { const list = Array.isArray(providers) ? providers : [providers]; for (const provider of list) { const keys = this.keysFor(provider.provide); @@ -172,6 +177,16 @@ export class Injector { return !!this.findRecord(token); } + /** + * Whether the token can actually produce a value. A record carrying only a + * pending loader answers `has()` but resolves to an error, so the deferred + * paths use this to tell "loaded and registered" from "loaded and silent". + */ + protected hasResolver(token: ProviderToken): boolean { + const found = this.findRecord(token); + return !!found && found.record.kind !== undefined; + } + /** First cached instance for a token, without triggering construction. */ public peek(token: ProviderToken): any { const found = this.findRecord(token); @@ -230,7 +245,10 @@ export class Injector { return name !== undefined ? [token, name] : [token]; } - private applyProvider(record: IProviderRecord, provider: Provider): void { + private applyProvider( + record: IProviderRecord, + provider: InternalProvider, + ): void { record.shared = provider.shared === undefined ? true : provider.shared; if ("useLazyRequire" in provider) { diff --git a/lib/common/di/providers.ts b/lib/common/di/providers.ts index 2395dfcb3d..42741eaaa8 100644 --- a/lib/common/di/providers.ts +++ b/lib/common/di/providers.ts @@ -37,7 +37,9 @@ export interface ILegacyClassProvider extends IBaseProvider { /** * Deferred side-effect loader (Yok's `require(name, path)`): running it is - * expected to register the real resolver onto this same record. + * expected to register the real resolver onto this same record. Container + * internals only — a record left with nothing but a loader resolves to an + * error, so it is deliberately kept out of `Provider`. */ export interface ILazyRequireProvider extends IBaseProvider { useLazyRequire: () => void; @@ -48,8 +50,10 @@ export type Provider = | IValueProvider | IFactoryProvider | ILazyClassProvider - | ILegacyClassProvider - | ILazyRequireProvider; + | ILegacyClassProvider; + +/** The provider forms the container accepts, including the unpublished ones. */ +export type InternalProvider = Provider | ILazyRequireProvider; /** Enforces at compile time that the implementation satisfies the token. */ export const provide = ( From 346c3857c79b8e2a48fb9b85c0ecdc1398de6e84 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 16:41:50 -0300 Subject: [PATCH 15/18] feat(commands): add registerDeferredCommand to the command registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claiming a command name and loading its implementation are now separate: the registry builds routing — the command record, the parent's subcommand list and the parent dispatcher — from the name alone, and runs the loader only when that one command is resolved. A sibling's dispatch no longer drags in the first claimant's module, and the outcome comes back as a structured result instead of a thrown message callers have to match on. Names that are not lower case are rejected: dispatch lower-cases what the user typed, so they could never be reached. A loader that throws, or that leaves the command without a resolver, fails naming the owner and the source. Extract registerDefinitionAs so a definition registered under a name chosen by its registrant is built exactly like one registered under its own. --- lib/common/contracts/command-registry.ts | 50 +++++++++- lib/common/contracts/index.ts | 5 + .../services/command-definition-adapter.ts | 29 ++++-- lib/common/yok.ts | 97 ++++++++++++++++++- 4 files changed, 168 insertions(+), 13 deletions(-) diff --git a/lib/common/contracts/command-registry.ts b/lib/common/contracts/command-registry.ts index 25ea0c8702..2f7f628798 100644 --- a/lib/common/contracts/command-registry.ts +++ b/lib/common/contracts/command-registry.ts @@ -1,6 +1,43 @@ import { Contract } from "../di/contract"; import type { ICommand } from "../definitions/commands"; +export interface DeferredCommandOptions { + /** + * Names the registrant in conflict and failure reports. Re-registering the + * same command under the same owner is a no-op rather than a conflict. + */ + owner: string; + /** Where the implementation comes from; named when loading it fails. */ + source: string; + /** + * Runs on first resolution of the command. It must leave a real resolver on + * the command name — by exporting a definition the caller registers, or by + * registering the command itself. + */ + load: () => void; +} + +/** Why a deferred registration did not take effect. */ +export type DeferredCommandRejection = + /** The name can never be dispatched; `detail` says why. */ + | { reason: "invalid-name"; detail: string } + /** Another owner registered the same command first. */ + | { reason: "claimed"; owner: string } + /** The CLI itself provides the command. */ + | { reason: "built-in" } + /** The name is in use as the dispatcher for subcommands under it. */ + | { reason: "subcommand-parent" }; + +/** + * Outcome of a deferred registration. Callers branch on `rejection.reason` + * rather than on message text, so the wording of the report stays theirs. + */ +export interface DeferredCommandResult { + registered: boolean; + /** Set exactly when `registered` is false. */ + rejection?: DeferredCommandRejection; +} + /** * The command-registry face of the injector facade. Transitional contract: it * mirrors what consumers call today, so that extracting the registry from the @@ -10,11 +47,20 @@ import type { ICommand } from "../definitions/commands"; @Contract({ name: "commandRegistry" }) export abstract class CommandRegistry { /** - * @deprecated Path-based command registration; slated for replacement by - * manifest-declared commands. + * @deprecated Path-based command registration; use registerDeferredCommand, + * which routes without loading and reports conflicts structurally. */ abstract requireCommand(names: string | string[], file: string): void; abstract registerCommand(names: string | string[], resolver: any): void; + /** + * Claims a command name for an owner without loading anything: routing — + * including the dispatcher of a hierarchical parent — is built from the name + * alone, and `load` runs only when that one command is resolved. + */ + abstract registerDeferredCommand( + name: string, + options: DeferredCommandOptions, + ): DeferredCommandResult; abstract resolveCommand(name: string): ICommand; abstract getRegisteredCommandsNames(includeDev: boolean): string[]; abstract getChildrenCommandsNames(commandName: string): string[]; diff --git a/lib/common/contracts/index.ts b/lib/common/contracts/index.ts index 3e0dafcdee..29e06c2109 100644 --- a/lib/common/contracts/index.ts +++ b/lib/common/contracts/index.ts @@ -5,6 +5,11 @@ // physically extracted — at which point the provider is swapped and consumers // keep working unchanged. export { CommandRegistry } from "./command-registry"; +export type { + DeferredCommandOptions, + DeferredCommandRejection, + DeferredCommandResult, +} from "./command-registry"; export { KeyCommandRegistry } from "./key-command-registry"; export { ModuleRegistry } from "./module-registry"; export { PublicApiBuilder } from "./public-api-builder"; diff --git a/lib/common/services/command-definition-adapter.ts b/lib/common/services/command-definition-adapter.ts index 92c85429b1..5ccefc5116 100644 --- a/lib/common/services/command-definition-adapter.ts +++ b/lib/common/services/command-definition-adapter.ts @@ -203,6 +203,26 @@ export function createCommandFromDefinition< }; } +/** + * Registers a definition under an externally chosen command name. Extension + * manifests route by their own key, which need not be the definition's own + * name, so the name is a parameter rather than read off the definition. + */ +export function registerDefinitionAs( + name: string, + definition: DefinedCommand, + targetInjector: IInjector = injector, +): 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), + ); +} + export function registerCommandDefinition( definition: DefinedCommand, targetInjector: IInjector = injector, @@ -214,18 +234,11 @@ export function registerCommandDefinition( ); } - // 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); const names = Array.isArray(definition.name) ? definition.name : [definition.name]; for (const name of names) { - // 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), - ); + registerDefinitionAs(name, definition, targetInjector); } } diff --git a/lib/common/yok.ts b/lib/common/yok.ts index 9a3c717ddc..0f4d77f45d 100644 --- a/lib/common/yok.ts +++ b/lib/common/yok.ts @@ -16,6 +16,11 @@ import { ModuleRegistry, PublicApiBuilder, } from "./contracts"; +import type { + DeferredCommandOptions, + DeferredCommandRejection, + DeferredCommandResult, +} from "./contracts"; /** * The legacy global facade binding. New code should obtain the container via @@ -24,6 +29,10 @@ import { */ export let injector: IInjector; +function rejected(rejection: DeferredCommandRejection): DeferredCommandResult { + return { registered: false, rejection }; +} + function forEachName(names: any, action: (name: string) => void): void { if (_.isString(names)) { action(names); @@ -95,10 +104,12 @@ export class Yok extends Injector implements IInjector { private placeholderParents = new Set(); private KEY_COMMANDS_NAMESPACE: string = "keyCommands"; private hierarchicalCommands: IDictionary = {}; + /** Deferred command name -> the owner that claimed it first. */ + private deferredCommandOwners: IDictionary = {}; /** - * @deprecated Path-based command registration; slated for replacement by - * manifest-declared commands. + * @deprecated Path-based command registration; use registerDeferredCommand, + * which routes without loading and reports conflicts structurally. */ public requireCommand(names: any, file: string): void { forEachName(names, (commandName) => { @@ -145,6 +156,86 @@ export class Yok extends Injector implements IInjector { }); } + public registerDeferredCommand( + name: string, + options: DeferredCommandOptions, + ): DeferredCommandResult { + if (name !== name.toLowerCase()) { + return rejected({ + reason: "invalid-name", + detail: + `command names are matched in lower case, so '${name}' can never ` + + `be dispatched; declare it as '${name.toLowerCase()}'`, + }); + } + + const claimedBy = this.deferredCommandOwners[name]; + if (claimedBy) { + return claimedBy === options.owner + ? { registered: true } + : rejected({ reason: "claimed", owner: claimedBy }); + } + + const commandRecordName = this.createCommandName(name); + if (this.has(commandRecordName)) { + return rejected( + this.synthesizedParents.has(name) + ? { reason: "subcommand-parent" } + : { reason: "built-in" }, + ); + } + + super.register({ + provide: commandRecordName, + useLazyRequire: () => { + try { + options.load(); + } catch (err) { + throw new Error( + `Unable to load command '${name}' of ${options.owner} from ` + + `${options.source}: ${err.message}`, + ); + } + + if (!this.hasResolver(commandRecordName)) { + throw new Error( + `Command '${name}' of ${options.owner} was not registered when ` + + `${options.source} loaded. The module must export a ` + + `defineCommand() definition or register the command itself.`, + ); + } + }, + }); + this.deferredCommandOwners[name] = options.owner; + + const commands = name.split(CommandsDelimiters.HierarchicalCommand); + if (commands.length > 1) { + const parentCommandName = commands[0]; + const subCommandName = _.tail(commands).join( + CommandsDelimiters.HierarchicalCommand, + ); + + if (!this.hierarchicalCommands[parentCommandName]) { + this.hierarchicalCommands[parentCommandName] = []; + } + + if ( + !_.includes( + this.hierarchicalCommands[parentCommandName], + subCommandName, + ) + ) { + this.hierarchicalCommands[parentCommandName].push(subCommandName); + } + + // The dispatcher routes off the recorded subcommand names alone, so + // reaching a sibling never loads this entry's module. + this.createHierarchicalCommand(parentCommandName, name); + } + + return { registered: true }; + } + /** * @deprecated Use provideLazy() from lib/common/di (via `Yok.di`) — the same * deferred loading, token-based. @@ -227,7 +318,7 @@ export class Yok extends Injector implements IInjector { // Yok replaced the whole record on an allowed re-require, dropping any // resolver and cached instances with it — preserved via remove(). this.remove(name); - this.register({ + super.register({ provide: name, useLazyRequire: () => require(dependencyPath), }); From e9f5b9ed0a9761183051c6287d533f6e62cd0201 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 16:42:06 -0300 Subject: [PATCH 16/18] feat(extensions): route manifest commands through the deferred registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The manifest loader no longer writes injector records or reads exception text to detect conflicts; it hands each entry to registerDeferredCommand and reports the rejection it gets back. A command claimed by another extension names that extension, one the CLI provides says so without exposing internals, and re-loading an already loaded extension is silent rather than a conflict with itself. Entry values may now be an object carrying the module path under `path`, with unrecognised keys ignored, so the shape can grow without stranding manifests on released CLIs. Default commands are registered ahead of their siblings so JSON key order carries no meaning. The manifest key is what the command is dispatched as — routing happens before the module exists — so a definition whose own name disagrees runs under the key and warns naming both, and definitions register through the same helper as registerCommandDefinition. --- lib/services/extensibility-service.ts | 171 ++++++++---- test/extension-manifests.ts | 358 +++++++++++++++++++++++++- 2 files changed, 465 insertions(+), 64 deletions(-) diff --git a/lib/services/extensibility-service.ts b/lib/services/extensibility-service.ts index 417eaf3481..e58daff365 100644 --- a/lib/services/extensibility-service.ts +++ b/lib/services/extensibility-service.ts @@ -6,6 +6,7 @@ import { createRegExp, regExpEscape } from "../common/helpers"; import { reportDeprecation } from "../common/deprecation"; import { INodePackageManager, INpmsSingleResultData } from "../declarations"; import { + IDictionary, IFileSystem, ISettingsService, IStringDictionary, @@ -22,8 +23,9 @@ import { IInjector } from "../common/definitions/yok"; import { CommandsDelimiters } from "../common/constants"; import { inject } from "../common/di/inject"; import { CommandRegistry } from "../common/contracts"; -import { isCommandDefinition } from "../common/define-command"; -import { createCommandFromDefinition } from "../common/services/command-definition-adapter"; +import type { DeferredCommandRejection } from "../common/contracts"; +import { DefinedCommand, isCommandDefinition } from "../common/define-command"; +import { registerDefinitionAs } from "../common/services/command-definition-adapter"; function isNonEmptyString(value: any): boolean { return typeof value === "string" && value.trim().length > 0; @@ -33,6 +35,44 @@ function isCommandsMap(commands: any): boolean { return !!commands && typeof commands === "object" && !Array.isArray(commands); } +/** + * A manifest entry is either the module path or an envelope carrying it under + * `path`. Unknown envelope keys are ignored on purpose: a CLI released today + * must keep loading manifests that grow new keys tomorrow. + */ +function getEntryModulePath(value: any): string { + if (isNonEmptyString(value)) { + return value; + } + + if ( + value && + typeof value === "object" && + !Array.isArray(value) && + isNonEmptyString(value.path) + ) { + return value.path; + } + + return null; +} + +const isDefaultCommandName = (name: string): boolean => + name.indexOf(CommandsDelimiters.DefaultHierarchicalCommand) !== -1; + +function describeRejection(rejection: DeferredCommandRejection): string { + switch (rejection.reason) { + case "invalid-name": + return rejection.detail; + case "claimed": + return `it is already registered by extension ${rejection.owner}`; + case "built-in": + return "it is already provided by the CLI"; + case "subcommand-parent": + return "it is already in use as the parent of its subcommands"; + } +} + /** * Reads the names of the commands an extension contributes out of either shape * of `nativescript.commands` - the legacy array of names, or the map of name to @@ -56,9 +96,6 @@ function getDeclaredCommandNames( export class ExtensibilityService implements IExtensibilityService { private customPathToExtensions: string = null; - /** Command name -> name of the extension whose manifest claimed it first. */ - private manifestCommandOwners: IStringDictionary = {}; - private commandRegistry = inject(CommandRegistry); private get pathToPackageJson(): string { @@ -316,11 +353,11 @@ export class ExtensibilityService implements IExtensibilityService { /** * Returns the `nativescript.commands` value of an extension only when it is a - * map of command name to module path. Any other shape (the legacy array of + * map of command name to module. Any other shape (the legacy array of * command names, a missing key, an unreadable package.json) yields null and * keeps the extension on the eager require path. */ - private getDeclaredCommandsMap(extensionName: string): IStringDictionary { + private getDeclaredCommandsMap(extensionName: string): IDictionary { let commands: any; try { @@ -340,7 +377,7 @@ export class ExtensibilityService implements IExtensibilityService { } /** - * Registers each declared command as a deferred require of its own module, so + * Registers each declared command as a deferred load of its own module, so * nothing from the extension is loaded until one of its commands is executed. * A module may either register itself on load (a legacy-style * `$injector.registerCommand(, )` at the top level) or export a @@ -350,69 +387,93 @@ export class ExtensibilityService implements IExtensibilityService { private registerDeclaredCommands( extensionName: string, pathToExtension: string, - commands: IStringDictionary, + commands: IDictionary, ): void { - for (const commandName of _.keys(commands)) { - const modulePath = commands[commandName]; + // Manifest key order carries no meaning, so a parent's default command is + // registered before its siblings rather than wherever the author put it. + const commandNames = _.sortBy(_.keys(commands), (commandName) => + isDefaultCommandName(commandName) ? 0 : 1, + ); + + for (const commandName of commandNames) { + const modulePath = getEntryModulePath(commands[commandName]); - if (!isNonEmptyString(commandName) || !isNonEmptyString(modulePath)) { + if (!isNonEmptyString(commandName) || !modulePath) { this.$logger.warn( `Extension ${extensionName} declares an invalid command in its nativescript.commands: '${commandName}': ${JSON.stringify( - modulePath, - )}. Both the command name and the path to its module must be non-empty strings. Skipping this command.`, + commands[commandName], + )}. The command name must be a non-empty string and its value either the path to its module or an object with a non-empty 'path'. Skipping this command.`, ); continue; } const absoluteModulePath = path.join(pathToExtension, modulePath); - const parentName = commandName.split( - CommandsDelimiters.HierarchicalCommand, - )[0]; - const parentWasAbsent = - parentName !== commandName && - !this.$injector.has(`commands.${parentName}`); + const result = this.commandRegistry.registerDeferredCommand(commandName, { + owner: extensionName, + source: absoluteModulePath, + load: () => + this.loadDeclaredCommand( + extensionName, + commandName, + absoluteModulePath, + ), + }); - try { - this.commandRegistry.requireCommand(commandName, absoluteModulePath); - } catch (err) { - const owner = this.manifestCommandOwners[commandName]; - const ownerInfo = owner - ? ` It is already registered by extension ${owner}.` - : ""; + if (!result.registered) { this.$logger.warn( - `Extension ${extensionName} is unable to register command ${commandName}.${ownerInfo} Error: ${err.message}`, + `Extension ${extensionName} is unable to register command '${commandName}': ${describeRejection( + result.rejection, + )}.`, ); - continue; } + } + } - // requireCommand's own loader only require()s the module for its side - // effects, which covers self-registering modules but not definition - // exports. The override must also land on a parent record this entry - // just created: dispatch resolves the parent BEFORE any child module - // has loaded, and the parent dispatcher only comes into existence once - // a child's registerCommand runs. - const loader = () => { - const exported = require(absoluteModulePath); - const candidate = (exported && exported.default) ?? exported; - if (isCommandDefinition(candidate)) { - this.commandRegistry.registerCommand(commandName, () => - createCommandFromDefinition(candidate), - ); - } - }; - this.$injector.register({ - provide: `commands.${commandName}`, - useLazyRequire: loader, - }); - if (parentWasAbsent) { - this.$injector.register({ - provide: `commands.${parentName}`, - useLazyRequire: loader, - }); - } + /** + * Runs on the first resolution of one declared command. Definition modules + * are registered here rather than by the module itself, which is what lets + * the manifest key stay authoritative for routing. + */ + private loadDeclaredCommand( + extensionName: string, + commandName: string, + absoluteModulePath: string, + ): void { + const exported = require(absoluteModulePath); + const candidate = (exported && exported.default) ?? exported; + + if (!isCommandDefinition(candidate)) { + return; + } + + this.warnOnDeclaredNameMismatch( + extensionName, + commandName, + absoluteModulePath, + candidate, + ); + registerDefinitionAs(commandName, candidate, this.$injector); + } - this.manifestCommandOwners[commandName] = extensionName; + private warnOnDeclaredNameMismatch( + extensionName: string, + commandName: string, + absoluteModulePath: string, + definition: DefinedCommand, + ): void { + const declaredNames = Array.isArray(definition.name) + ? definition.name + : [definition.name]; + + if (_.includes(declaredNames, commandName)) { + return; } + + this.$logger.warn( + `Extension ${extensionName} declares command '${commandName}' in its package.json, but the definition in ${absoluteModulePath} names itself '${declaredNames.join( + "', '", + )}'. The command runs as '${commandName}' - the manifest decides how it is invoked.`, + ); } private getPathToExtension(extensionName: string): string { diff --git a/test/extension-manifests.ts b/test/extension-manifests.ts index 4bfd387d44..ef7c3cf684 100644 --- a/test/extension-manifests.ts +++ b/test/extension-manifests.ts @@ -461,10 +461,14 @@ describe("extension manifests", () => { describe("commands declared as defineCommand modules", () => { const contractsPath = require.resolve("../lib/contracts"); - const definitionModule = (commandName: string, marker: string): string => + const definitionModule = ( + commandName: string, + marker: string, + exportAs: string = "module.exports", + ): string => `const { defineCommand } = require(${JSON.stringify(contractsPath)}); global.__nsmCapture.loadedModules.push(${JSON.stringify(marker)}); - module.exports = defineCommand({ + ${exportAs} = defineCommand({ name: ${JSON.stringify(commandName)}, arguments: "any", async run(ctx) { @@ -500,29 +504,365 @@ describe("extension manifests", () => { ]); }); - it("resolves the hierarchical parent dispatcher before any child module has loaded", async () => { + it("resolves the hierarchical parent dispatcher without loading any child module", async () => { const extensionName = "nsm-defp-ext"; writeExtension( extensionName, - { commands: { "nsmdefp|go": "./dist/go.js" } }, + { + commands: { + "nsmdefp|go": "./dist/go.js", + "nsmdefp|stop": "./dist/stop.js", + }, + }, { "main.js": mainModule("defp-main"), "dist/go.js": definitionModule("nsmdefp|go", "defp-go"), + "dist/stop.js": definitionModule("nsmdefp|stop", "defp-stop"), }, ); const extensibilityService = resolveService(testInjector); await extensibilityService.loadExtension(extensionName); - // Dispatch hits the parent first; its record must load the child module - // so the dispatcher the child's registration synthesizes exists. const parent = testInjector.resolveCommand("nsmdefp"); assert.isOk(parent); assert.isTrue(parent.isHierarchicalCommand); - assert.include(capture.loadedModules, "defp-go"); + assert.deepEqual(capture.loadedModules, []); + assert.deepEqual(testInjector.getChildrenCommandsNames("nsmdefp"), [ + "go", + "stop", + ]); + + assert.isOk(testInjector.resolveCommand("nsmdefp|go")); + assert.deepEqual(capture.loadedModules, ["defp-go"]); + }); + + it("registers a definition under the manifest key and warns about a disagreeing name", async () => { + const extensionName = "nsm-defname-ext"; + writeExtension( + extensionName, + { commands: { "nsmdefname|run": "./dist/run.js" } }, + { + "main.js": mainModule("defname-main"), + "dist/run.js": definitionModule("something|else", "defname-run"), + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + const command = testInjector.resolveCommand("nsmdefname|run"); + assert.isOk(command); + await command.execute([]); + assert.deepEqual(capture.executed, [{ marker: "defname-run", args: [] }]); + assert.isNull(testInjector.resolveCommand("something|else")); + + const warnOutput = getLogger(testInjector).warnOutput; + assert.include(warnOutput, "nsmdefname|run"); + assert.include(warnOutput, "something|else"); + assert.include(warnOutput, extensionName); + }); + + it("adapts a definition exported as the module's default", async () => { + const extensionName = "nsm-defdefault-ext"; + writeExtension( + extensionName, + { commands: { "nsmdefdefault|hello": "./dist/hello.js" } }, + { + "main.js": mainModule("defdefault-main"), + "dist/hello.js": definitionModule( + "nsmdefdefault|hello", + "defdefault-hello", + "exports.default", + ), + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + const command = testInjector.resolveCommand("nsmdefdefault|hello"); + assert.isOk(command); + await command.execute([]); + assert.deepEqual(capture.executed, [ + { marker: "defdefault-hello", args: [] }, + ]); + }); + + it("routes two aliases of one command to the same module", async () => { + const extensionName = "nsm-alias-ext"; + writeExtension( + extensionName, + { + commands: { + "nsmalias|run": "./dist/run.js", + "nsmalias|r": "./dist/run.js", + }, + }, + { + "main.js": mainModule("alias-main"), + "dist/run.js": definitionModule("nsmalias|run", "alias-run"), + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + assert.isOk(testInjector.resolveCommand("nsmalias|run")); + const aliased = testInjector.resolveCommand("nsmalias|r"); + assert.isOk(aliased); + + await aliased.execute(["x"]); + assert.deepEqual(capture.executed, [ + { marker: "alias-run", args: ["x"] }, + ]); + }); + }); + + describe("manifest entry values", () => { + it("accepts an object entry carrying the module path and ignores its other keys", async () => { + const extensionName = "nsm-envelope-ext"; + writeExtension( + extensionName, + { + commands: { + "nsmenvelope|run": { + path: "./run.js", + somethingAddedLater: true, + }, + }, + }, + { + "main.js": mainModule("envelope-main"), + "run.js": commandModule("nsmenvelope|run", "envelope-run"), + }, + ); - const child = testInjector.resolveCommand("nsmdefp|go"); - assert.isOk(child); + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + assert.deepEqual(capture.loadedModules, []); + assert.isOk(testInjector.resolveCommand("nsmenvelope|run")); + assert.deepEqual(capture.loadedModules, ["envelope-run"]); + }); + + it("warns about and skips an object entry without a usable path", async () => { + const extensionName = "nsm-envelope-bad-ext"; + writeExtension( + extensionName, + { commands: { "nsmenvelopebad|run": { module: "./run.js" } } }, + { "main.js": mainModule("envelope-bad-main") }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + assert.include(getLogger(testInjector).warnOutput, "nsmenvelopebad|run"); + assert.isNull(testInjector.resolveCommand("nsmenvelopebad|run")); + }); + }); + + describe("manifest keys the CLI cannot route", () => { + it("rejects a key that is not lower case", async () => { + const extensionName = "nsm-case-ext"; + writeExtension( + extensionName, + { commands: { "nsmCase|Run": "./run.js" } }, + { + "main.js": mainModule("case-main"), + "run.js": commandModule("nsmcase|run", "case-run"), + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + const warnOutput = getLogger(testInjector).warnOutput; + assert.include(warnOutput, "nsmCase|Run"); + assert.include(warnOutput, "nsmcase|run"); + assert.include(warnOutput, extensionName); + assert.isNull(testInjector.resolveCommand("nsmCase|Run")); + assert.isNull(testInjector.resolveCommand("nsmcase|run")); + }); + + it("rejects a key already in use as the parent of its own subcommands", async () => { + const extensionName = "nsm-parentclash-ext"; + writeExtension( + extensionName, + { + commands: { + "nsmparentclash|run": "./run.js", + nsmparentclash: "./flat.js", + }, + }, + { + "main.js": mainModule("parentclash-main"), + "run.js": commandModule("nsmparentclash|run", "parentclash-run"), + "flat.js": commandModule("nsmparentclash", "parentclash-flat"), + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + const warnOutput = getLogger(testInjector).warnOutput; + assert.include(warnOutput, "nsmparentclash"); + assert.include(warnOutput, "parent of its subcommands"); + + const parent = testInjector.resolveCommand("nsmparentclash"); + assert.isTrue(parent.isHierarchicalCommand); + assert.deepEqual(capture.loadedModules, []); + }); + + it("reports a command the CLI itself provides without naming internals", async () => { + const extensionName = "nsm-builtin-ext"; + class BuiltInCommand { + public allowedParameters: any[] = []; + public async execute(): Promise { + return undefined; + } + } + testInjector.registerCommand("nsmbuiltin", BuiltInCommand); + + writeExtension( + extensionName, + { commands: { nsmbuiltin: "./run.js" } }, + { + "main.js": mainModule("builtin-main"), + "run.js": commandModule("nsmbuiltin", "builtin-run"), + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + const warnOutput = getLogger(testInjector).warnOutput; + assert.include(warnOutput, "already provided by the CLI"); + assert.include(warnOutput, extensionName); + assert.notInclude(warnOutput, "commands."); + + assert.instanceOf( + testInjector.resolveCommand("nsmbuiltin"), + BuiltInCommand, + ); + assert.deepEqual(capture.loadedModules, []); + }); + }); + + describe("loading an already loaded extension", () => { + it("does not report the extension as conflicting with itself", async () => { + const extensionName = "nsm-reload-ext"; + writeExtension( + extensionName, + { commands: { "nsmreload|run": "./run.js" } }, + { + "main.js": mainModule("reload-main"), + "run.js": commandModule("nsmreload|run", "reload-run"), + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + await extensibilityService.loadExtension(extensionName); + + assert.equal(getLogger(testInjector).warnOutput, ""); + assert.isOk(testInjector.resolveCommand("nsmreload|run")); + }); + }); + + describe("a manifest declaring no commands to load", () => { + it("loads nothing at all for an empty commands map", async () => { + const extensionName = "nsm-optout-ext"; + writeExtension( + extensionName, + { commands: {} }, + { "main.js": mainModule("optout-main") }, + ); + + const extensibilityService = resolveService(testInjector); + const extensionData = + await extensibilityService.loadExtension(extensionName); + + assert.deepStrictEqual(requiredPaths, []); + assert.deepStrictEqual(capture.loadedModules, []); + assert.notInclude(getLogger(testInjector).traceOutput, DEPRECATION_API); + assert.deepStrictEqual(extensionData.commands, []); + }); + }); + + describe("a module that fails to provide its command", () => { + it("names the extension and the module when the module throws", async () => { + const extensionName = "nsm-throwing-ext"; + writeExtension( + extensionName, + { commands: { "nsmthrowing|run": "./run.js" } }, + { + "main.js": mainModule("throwing-main"), + "run.js": `throw new Error("kaboom");`, + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + assert.throws( + () => testInjector.resolveCommand("nsmthrowing|run"), + /nsmthrowing\|run[\s\S]*nsm-throwing-ext[\s\S]*run\.js[\s\S]*kaboom/, + ); + }); + + it("names the extension and the module when the module registers nothing", async () => { + const extensionName = "nsm-silent-ext"; + writeExtension( + extensionName, + { commands: { "nsmsilent|run": "./run.js" } }, + { + "main.js": mainModule("silent-main"), + "run.js": `module.exports = { notADefinition: true };`, + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + assert.throws( + () => testInjector.resolveCommand("nsmsilent|run"), + /nsmsilent\|run[\s\S]*nsm-silent-ext[\s\S]*run\.js/, + ); + }); + }); + + describe("default commands", () => { + it("registers the default before its siblings whatever the key order", async () => { + const extensionName = "nsm-defaults-ext"; + writeExtension( + extensionName, + { + commands: { + "nsmdefaults|other": "./other.js", + "nsmdefaults|*default": "./default.js", + }, + }, + { + "main.js": mainModule("defaults-main"), + "other.js": commandModule("nsmdefaults|other", "defaults-other"), + "default.js": commandModule( + "nsmdefaults|*default", + "defaults-default", + ), + }, + ); + + const extensibilityService = resolveService(testInjector); + await extensibilityService.loadExtension(extensionName); + + assert.equal(getLogger(testInjector).warnOutput, ""); + assert.deepEqual(testInjector.getChildrenCommandsNames("nsmdefaults"), [ + "*default", + "other", + ]); + assert.isOk(testInjector.resolveCommand("nsmdefaults|*default")); + assert.deepEqual(capture.loadedModules, ["defaults-default"]); }); }); }); From bcd08f0d56ec962552cbe8b8257e8d4fa819197b Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 16:42:07 -0300 Subject: [PATCH 17/18] docs(extensions): document the manifest contract and the CLI dependency Lead with the peerDependency + devDependency pair that makes `require("nativescript/contracts")` resolve and keeps a second CLI copy out of the tree, and teach inject() as the way to reach a CLI service. Cover what the manifest actually promises: the key is authoritative for routing, aliases are duplicate entries pointing at one module, entry values may be envelopes, an empty map opts out of loading, keys must be lower case, and "first" in first-wins is the order extensions load in. Drop the JSON key-order constraint, which no longer exists. --- extensions.md | 157 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 132 insertions(+), 25 deletions(-) diff --git a/extensions.md b/extensions.md index b8feb9a035..074d262c07 100644 --- a/extensions.md +++ b/extensions.md @@ -2,8 +2,7 @@ Writing a CLI Extension ======================= An extension adds new commands to the NativeScript CLI. Extensions are ordinary -npm packages: they are published to npm, installed per user rather than per -project, and are available from every project on the machine. +npm packages. ```bash ns extension install @@ -16,20 +15,50 @@ of them. That makes the manifest below the most important file in an extension: it is what the CLI reads on startup, and it decides whether your code is loaded eagerly or only when one of your commands is actually executed. -## Making a package discoverable +## Depending on the CLI -Add the `nativescript:extension` keyword to `package.json`. The CLI searches npm -for that keyword when it needs to suggest an extension for an unknown command -(see [Suggesting an extension](#suggesting-an-extension-for-an-unknown-command)). +An extension that imports anything from the CLI — `defineCommand`, `inject`, the +types — needs `nativescript` declared twice: ```json { "name": "nativescript-hello", "version": "1.0.0", - "keywords": ["nativescript:extension"] + "keywords": ["nativescript:extension"], + "peerDependencies": { + "nativescript": ">=9.1.0" + }, + "devDependencies": { + "nativescript": "^9.1.0" + } } ``` +- The **peer dependency** declares which CLI versions the extension works with, + and keeps package managers from installing a second copy of the CLI next to + your extension. Your code must run against the _running_ CLI: a second copy + brings its own injector, and services resolved from it are not the ones + executing the command. +- The **dev dependency** is what makes `require("nativescript/contracts")` + resolve while you build and test the extension. It is not installed for your + users. + +Never declare `nativescript` as a plain dependency. + +`nativescript/contracts` is the entry point extensions import from. It is +side-effect free — importing it does not boot a CLI — and it exports +`defineCommand`, `inject`, the option helpers and the public types. + +The `nativescript:extension` keyword makes the package discoverable: the CLI +searches npm for it when it needs to suggest an extension for an unknown command +(see [Suggesting an extension](#suggesting-an-extension-for-an-unknown-command)). + +> Extensions are installed per user today, and are available from every project +> on the machine. Installing them as project `devDependencies` — pinned per +> project, reproducible in CI, shared with the team — is the direction this is +> heading; declaring the peer dependency now is what makes an extension ready +> for it. + ## Declaring commands Commands are declared in the `commands` key of the `nativescript` key of the @@ -48,31 +77,63 @@ extension's `package.json`. Two shapes are accepted. } ``` -Each key is a command name; each value is a path to the module implementing it, -resolved relative to the extension's root directory. +Each key is a command name; each value says where the module implementing it +lives, resolved relative to the extension's root directory. A value is either +the path itself or an object carrying it under `path`: + +```json +{ + "nativescript": { + "commands": { + "hello|world": { "path": "./dist/commands/hello-world.js" } + } + } +} +``` + +The two forms mean exactly the same thing today. Keys the CLI does not +recognise inside the object form are ignored, so the object can carry +information a later CLI understands without breaking the one you have +installed. Declaring commands this way is strongly preferred: -* **Per-command lazy loading.** Nothing in the extension is loaded when the CLI +- **Per-command lazy loading.** Nothing in the extension is loaded when the CLI starts. A command's module is required the first time that command is resolved, so `ns build android` never pays the cost of loading an unrelated extension. With a large or dependency-heavy extension installed, that is the difference between a noticeable startup delay on every command and none. -* **Early, named conflict detection.** Two extensions claiming the same command + Dispatching `ns hello world` loads only `hello-world.js` — not the sibling + `hello.js`, and not the extension's main entry. +- **Early, named conflict detection.** Two extensions claiming the same command name is reported as a warning that names both extensions and the contested command, and the extension that claimed it first keeps working. Under the legacy shape the same collision surfaces as an opaque `module '...' require'd twice.` failure from whichever extension happened to load second. -* **The CLI knows what you contribute without running you.** The declared +- **The CLI knows what you contribute without running you.** The declared command names are what the install suggestion for an unknown command matches against, and they are available to the CLI as metadata about the installed extension. -Malformed entries are skipped rather than fatal: an entry whose command name or -module path is not a non-empty string is reported as a warning naming the -extension and the offending entry, and the extension's remaining commands are -still registered. +Malformed entries are skipped rather than fatal: an entry whose command name is +not a non-empty string, or whose value carries no usable module path, is +reported as a warning naming the extension and the offending entry, and the +extension's remaining commands are still registered. + +An empty map opts out of loading entirely: + +```json +{ + "nativescript": { + "commands": {} + } +} +``` + +The extension contributes no commands, and — unlike omitting the key — its main +entry is never required. Use it for an extension that only ships documentation +or assets. ### An array of command names (legacy) @@ -86,10 +147,10 @@ still registered. The array is a discovery aid only — it lists the names the CLI may suggest your extension for, but it says nothing about where the implementations live. An -extension declaring commands this way (or declaring no commands at all) is -loaded the old way: the CLI `require()`s the package's main entry on **every** -invocation and expects the module's top-level code to register everything -through the injector global. +extension declaring commands this way (or omitting the `commands` key +altogether) is loaded the old way: the CLI `require()`s the package's main entry +on **every** invocation and expects the module's top-level code to register +everything through the injector global. This path remains supported for published extensions, but it is tracked for eventual deprecation and new extensions should not use it — declare the map @@ -117,6 +178,14 @@ module.exports = defineCommand({ }); ``` +`inject()` resolves a CLI service against the injector running the command, and +works anywhere inside `run` up to the first `await`. It is why the peer +dependency above matters: with a second copy of the CLI installed alongside your +extension, `inject()` warns and points at the duplicate. + +A definition exported as `module.exports.default` (what a TypeScript or ESM +build emits) is picked up too. + Legacy modules — command classes that register themselves at load time through the injector global, with parameter-name constructor injection — keep working when a manifest entry points at them, so existing extensions can adopt the map @@ -124,17 +193,55 @@ without rewriting their commands. Both of those mechanisms are deprecated (see [dependency-injection.md](dependency-injection.md)); write new modules as definitions. +If a module named by a manifest entry neither exports a definition nor registers +the command itself, executing that command fails with an error naming the +extension, the command and the module — the entry points at the wrong file, or +the file is not doing what the entry promises. + ## Command names Command names use `|` to express hierarchy, so `"hello|world"` is invoked as `ns hello world`. Prefixing the last segment with `*` marks a default subcommand: `"hello|*default"` runs both for `ns hello default` and for a bare -`ns hello`. +`ns hello`. Names must be lower case — the CLI matches what the user typed in +lower case, so a key with an upper-case letter could never be reached, and is +rejected with a warning. + +**The manifest key decides how a command is invoked.** It has to: the CLI routes +`ns hello world` to your module before that module has been loaded, so the key +is the only name it can know. A `name` inside the definition is metadata — it is +what `registerCommandDefinition` uses when a module registers itself, and it is +useful documentation, but a manifest entry overrides it. If the two disagree the +CLI warns, naming both, and runs the command under the manifest key. + +An alias is a second entry pointing at the same module: + +```json +{ + "nativescript": { + "commands": { + "hello|world": "./dist/commands/hello-world.js", + "hello|w": "./dist/commands/hello-world.js" + } + } +} +``` + +Both names route to the same module, which is loaded once. + +## When two extensions want the same command + +The first extension to claim a command name keeps it; later claimants are +reported with a warning naming both extensions and the command, and their entry +is skipped. A name the CLI itself provides is never taken over — the extension +is told the command is already provided by the CLI. -When an extension contributes several commands under the same parent, declare -the default command before its siblings — the CLI creates the parent dispatcher -from the first entry it sees, and a default command registered after that parent -already exists is rejected. +"First" is the order extensions are loaded in, which is the order they appear in +the `dependencies` of the profile directory's `extensions/package.json` — npm +keeps that alphabetically sorted, so in practice the alphabetically first +extension name wins. The exception is `ns extension install`: that invocation +loads the freshly installed extension after all the others, so a conflict it +would win on the next invocation goes the other way that one time. ## Suggesting an extension for an unknown command From 13c9f1f246707be7eb7b5e534523e545359e6b26 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 17:19:56 -0300 Subject: [PATCH 18/18] docs(extensions): note how prerelease CLIs interact with the peer range --- extensions.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/extensions.md b/extensions.md index 074d262c07..2c19680b09 100644 --- a/extensions.md +++ b/extensions.md @@ -42,6 +42,10 @@ types — needs `nativescript` declared twice: - The **dev dependency** is what makes `require("nativescript/contracts")` resolve while you build and test the extension. It is not installed for your users. +- Developing against a **prerelease** CLI? Semver ranges without a prerelease + tag never match one — `9.1.0-alpha.15` does not satisfy `>=9.1.0` — so pin + the exact prerelease as your dev dependency and keep the stable floor in + `peerDependencies`. Never declare `nativescript` as a plain dependency.