Skip to content

feat(commands): defineCommand — typed declarative commands via an ICommand adapter - #6101

Draft
edusperoni wants to merge 19 commits into
mainfrom
feat/define-command
Draft

feat(commands): defineCommand — typed declarative commands via an ICommand adapter#6101
edusperoni wants to merge 19 commits into
mainfrom
feat/define-command

Conversation

@edusperoni

Copy link
Copy Markdown
Collaborator

Stacked on #6099 (feat/di-modernization-phase1).

PR Checklist

What is the current behavior?

A command is a class implementing ICommand, registered under a stringly-typed key, reading flags off the untyped global $options object. The validation semantics carry a trap: declaring canExecute silently disables allowedParameters validation, and an empty allowedParameters means "reject all positional arguments" — none of which the types express.

What is the new behavior?

defineCommand — a declarative, typed command definition that plugs into the existing registry through an adapter (createCommandFromDefinition / registerCommandDefinition). Fully additive: routing, validation, help, hooks, and analytics behavior are untouched, and legacy ICommand classes remain fully supported.

export default defineCommand({
	name: "widget|add",
	options: { verbose: booleanOption({ default: false }), output: stringOption({ alias: "o" }) },
	arguments: "none",
	async run(ctx) {
		// ctx.options.verbose is a boolean — inferred from the schema
	},
});
  • Option schemas compile to dashedOptions, riding the CLI's existing option validation (declared flags accepted; unknown flags hard-fail with help, exactly as today). ctx.options is a typed view with real inference (pinned by an exactness type test, not a strict-off-friendly assignment).
  • The canExecute trap is handled explicitly and documented as a constraint in the adapter: arguments: "none" (default) omits canExecute so the framework's own no-parameters validation applies; arguments: "any" or a user canExecute takes validation ownership wholesale.
  • run executes in an injection context, so inject() works inside commands the same as everywhere else.
  • lib/common/define-command.ts is side-effect-free and exported from nativescript/contracts; definitions carry a Symbol.for marker so duplicated CLI copies interoperate.
  • 18 new tests, including two end-to-end through CommandsService.tryExecuteCommand.
  • New authoring guide: defining-commands.md.

Two pre-existing registry quirks surfaced while testing, unchanged by this PR and worth their own issues: the synthesized hierarchical parent registers via the module-level global injector rather than the instance it was called on, and registerCommand alone never populates hierarchicalCommands routing state (only requireCommand does).

Full suite: 110 files, 1616 passed / 38 skipped; yok oracle, public-API test, and compat fixtures untouched.

Purpose-built container with an Angular-compatible surface (inject,
runInInjectionContext, Injector, provide/provideLazy, forwardRef).
Lookup is class-object first with a fallback to the decorator-set name,
per injector level, so per-call string overrides and duplicated contract
copies in the extensions tree resolve to the same provider. Includes a
legacy provider kind that constructs Yok-style classes via annotate(),
lazy side-effect loaders for path-based registration, transient
retention, and reverse-instantiation-order disposal.
reportDeprecation() dedups per api+detail and logs at trace level by
default; NS_DEPRECATIONS=warn|error previews the stricter stages so the
same call sites can be escalated over releases. Wired at the external
entry points only: param-name hook invocation, require-time extension
registration, and dynamicCall help templating.
A class migrated off property injection has neither $hooksService nor
$injector, so the decorator threw at hook-execution time. The global
injector must stay last in the chain - tests stub the instance
properties and rely on them winning.
Yok keeps its entire public surface, subclassability, and the global
$injector, while storage and resolution delegate to the new container.
Command routing, key commands, and the public-API builder are unchanged.
Every legacy member now carries @deprecated JSDoc naming its
replacement, mirrored on IInjector.
DoctorService and ProjectNameService become @contract abstract classes;
their impls are renamed *Impl (externally invisible - outside resolution
is by string name or token, never class identity). The subpath resolves
through contracts/package.json rather than an exports map, so existing
deep requires keep working, and the entry point is side-effect-free so a
duplicated CLI copy in an extensions tree never boots a second runtime.
Fixtures exercise the surfaces third parties rely on: param-name hook
signatures across every payload shape and influence channel (mutation,
function-return middleware, abort), require-time extension registration
against global.$injector including hierarchical commands, and the full
IInjector facade surface. Notably they pin that a hook naming an
unwrapped payload key (the after-watchAction shape) is skipped as
invalid - resolution changes must not resurrect long-dead hooks.
dependency-injection.md covers tokens (@contract), inject()/Injector,
provider kinds, resolution semantics, forwardRef, coexistence with the
legacy $injector, and a legacy-to-new quick reference. extending-cli.md
leads with the recommended hook pattern - inject() works directly in
hook bodies because they run in an injection context, pinned by a new
compat test - and demotes parameter-name injection to a labeled legacy
section. String-token lookups are framed as the migration bridge, not a
co-equal API.

The hook deprecation tracer now flags only hooks that actually use
param-name service injection; a hookArgs-only signature follows the
recommended pattern and stays silent.
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 23dc3228-9431-4bc8-9319-adc51984694e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

createHierarchicalCommand registered the synthesized parent - and its
execute path resolved commandsService and errors - through the
module-level global injector instead of the instance it was called on,
so a parent dispatcher leaked onto the global injector whenever a
hierarchical command was registered on any other instance.

The require-ordering guard accidentally depended on that leak: with the
parent now landing on the same instance, it is exempted for synthesized
parents (they exist because a child registered, not because requires ran
out of order), and the else branch mirrors the existing default-command
skip. Net effect: register-then-require orderings that used to throw
'Default commands should be required before child commands' now work;
nothing that worked before changes.
Internal code no longer reads global.$injector: the two cycle-bound
sites (the deprecation tracer's logger fallback and @hook's last-resort
lookup) go through a getInjector() accessor required at call time. The
global property remains write-only from the CLI's side - it exists as
the published surface for extensions and hooks.
@edusperoni
edusperoni force-pushed the feat/define-command branch from b15734b to 863f964 Compare July 30, 2026 01:39
The token container was reachable only through a getter on the concrete
Yok class, so every crossing from facade-typed code into the new API
needed an any cast - the migration's most important seam was invisible
to the type system. IInjector now declares readonly di: Injector.
@edusperoni
edusperoni force-pushed the feat/define-command branch from 863f964 to 74caa8f Compare July 30, 2026 01:44
Yok is now an Injector - class Yok extends Injector - so the new API
works on the facade directly (get, register with Providers, createChild,
runInInjectionContext($injector, ...)) and inject(Injector) inside
legacy-constructed classes returns the facade itself instead of a
second, inner container identity. The di bridge is gone. register()
dispatches by argument shape: a string first argument is the legacy
name-based form, anything else is a Provider.

IInjector now extends the Injector class type, which constrains
implementers to the real class hierarchy (Yok and its subclasses) -
intentional, since the interface only ever described Yok.
IInjector recomposes from per-subsystem faces - CommandRegistry,
KeyCommandRegistry, ModuleRegistry, PublicApiBuilder - each an @contract
token the facade registers itself under. One object still implements
everything until the subsystems are physically extracted; extraction
then becomes a provider swap for the face's token instead of a consumer
migration. Consumers can depend on the narrow face they actually use,
and deprecation becomes per-face instead of a flat everything-is-legacy.

The contracts are internal (lib/common/contracts) and deliberately not
re-exported from nativescript/contracts; promoting one is a per-contract
decision.
- doctor-service: printWarnings accepts an optional trackResult matching
  its contracts; runSetupScript returns the setup script result instead
  of resolving undefined; canExecuteLocalBuild no longer dereferences
  its optional argument
- deprecation tracer: a report dropped for lack of a logger is no
  longer latched as delivered - it reports once a logger exists
- yok: global.$injector is an accessor pair, so a direct third-party
  assignment stays synchronized with the binding getInjector() reads
- docs: note the useValue + shared:false quirk; show where Injector is
  imported from in the late-lookup example
@edusperoni
edusperoni force-pushed the feat/define-command branch from 74caa8f to cafa737 Compare July 30, 2026 02:27
optional resolves to null instead of throwing for unknown tokens (a
found-but-misconfigured provider still throws); skipSelf starts at the
parent, escaping a child scope's shadowing entry; self refuses parent
fallthrough. self+skipSelf throws. host is deliberately absent - it is
an Angular component-tree concept with no analog in this hierarchy.

get()'s former second parameter - the legacy per-call ctorArguments bag
- had exactly one caller, the facade's own resolve(name, bag). It moves
to a protected getWithLegacyArguments channel, so the public get()
aligns with Angular's shape today rather than after the legacy paths
are deleted.
…adapter

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.
…nitions

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.
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.
Yok extends Injector on the base branch; the di bridge is gone.
@edusperoni
edusperoni force-pushed the feat/define-command branch from cafa737 to a1ba0ef Compare July 30, 2026 02:51
Base automatically changed from feat/di-modernization-phase1 to main August 3, 2026 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant