Skip to content

feat(extensions): nativescript.commands map — per-command lazy loading for extensions - #6102

Open
edusperoni wants to merge 19 commits into
mainfrom
feat/extension-manifests
Open

feat(extensions): nativescript.commands map — per-command lazy loading for extensions#6102
edusperoni wants to merge 19 commits into
mainfrom
feat/extension-manifests

Conversation

@edusperoni

@edusperoni edusperoni commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #6101 (feat/define-command); #6099 (the DI foundation) is merged into main.

PR Checklist

What is the current behavior?

Every installed extension is eagerly require()d on every CLI invocation, before the command is even known — the extension's whole module tree loads so its top-level side effects can register commands against global.$injector. nativescript.commands in an extension's package.json is a string[] used only to suggest installs for unknown commands. Two extensions claiming the same command name crash at startup.

What is the new behavior?

nativescript.commands also accepts a map of command name → module path, which becomes authoritative:

"nativescript": {
	"commands": {
		"widget|add": "./dist/commands/widget-add.js",
		"widget|new": { "path": "./dist/commands/widget-add.js" }  // alias: same module, second entry
	}
}
  • Per-command lazy loading: nothing from the extension loads until one of its commands actually executes; the extension main is never required, and map-manifest extensions are not flagged by the deprecation tracer (the legacy array/eager path keeps working verbatim, tracer included). Values accept string | { path } so the envelope can grow additively.
  • The manifest key is authoritative: routing works before any module loads. If a loaded defineCommand definition's name disagrees with its manifest key, the CLI warns naming both and runs under the key. Aliases are duplicate manifest entries pointing at the same module.
  • registerDeferredCommand on the CommandRegistry facet: claiming a name and loading its implementation are now separate registry operations. The registry builds the command record, the parent's subcommand list, and the parent dispatcher from the name alone — a sibling's dispatch never drags in the first claimant's module — and returns a structured DeferredCommandResult (claimed / built-in / subcommand-parent / invalid-name) instead of exception text callers must match on. This is what keeps the future registry extraction a provider swap.
  • Failure UX: loader failures name the command, the owning extension, and the module path; a loader that runs but registers nothing fails the same way; non-lowercase manifest keys (permanently unreachable — dispatch lower-cases input) warn and are skipped without sinking the extension's other commands. Built-in conflicts say "already provided by the CLI", no internals.
  • Deterministic defaults: *default entries sort first per parent in code — JSON key order carries no meaning. First-wins conflict resolution is defined in the docs (extension load order, alphabetical; the mid-load ns extension install exception documented). Re-declaring a command under the same owner is a no-op, so ns extension install <already-installed> no longer warns about conflicting with itself. "commands": {} opts out of loading entirely.
  • A command module may self-register on load (legacy shape) or simply export a defineCommand definition — one registration code path (registerDefinitionAs) serves both the manifest loader and registerCommandDefinition.
  • ILazyRequireProvider is no longer part of the exported Provider union (container-internal).
  • New authoring guide: extensions.md — leads with the peerDependency + devDependency on nativescript and inject() from nativescript/contracts.

Public type names follow the new-API convention (no I prefix): DeferredCommandOptions, DeferredCommandResult, DeferredCommandRejection.

25 tests in test/extension-manifests.ts (lazy registration, eager-path preservation, malformed/conflict/self-conflict handling, both suggestion shapes, pure-definition modules incl. resolving the parent dispatcher before any child module has loaded, key-mismatch warning, alias entries, {} opt-out). Full stacked suite: 116 files, 1784 passed / 9 skipped; yok oracle, public-API test, and compat fixtures untouched.

Summary by CodeRabbit

  • New Features

    • Extensions can declare CLI commands directly in their manifests.
    • Commands are loaded on demand, with support for command definitions and aliases.
    • Added validation and clear handling for naming conflicts, malformed entries, and loading failures.
    • Extension metadata now reports declared commands.
    • Added comprehensive documentation for CLI extensions and command configuration.
  • Documentation

    • Added related-guide links and updated command declaration guidance.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The CLI adds deferred command registration for extension manifest maps. It validates command names, resolves ownership conflicts, loads modules lazily, adapts exported command definitions, preserves legacy arrays, and documents the supported extension formats.

Changes

Declarative Extension Commands

Layer / File(s) Summary
Command contracts and container providers
lib/common/contracts/..., lib/common/di/..., lib/common/definitions/extensibility.d.ts
Adds deferred command contracts, internal provider types, resolver detection, and extension command metadata.
Deferred command registry
lib/common/yok.ts
Adds name validation, ownership tracking, conflict handling, lazy loading, structured failures, and hierarchical command wiring.
Manifest command loading
lib/common/services/command-definition-adapter.ts, lib/services/extensibility-service.ts
Registers command maps lazily, supports command definitions and legacy self-registration, preserves array manifests, and reports mismatches.
Validation and extension documentation
test/extension-manifests.ts, extensions.md, defining-commands.md, dependency-injection.md
Adds manifest and loading tests and documents installation, command formats, conflicts, aliases, defaults, and command help configuration.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ExtensionManifest
  participant ExtensibilityService
  participant CommandRegistry
  participant LazyCommandModule
  participant CommandDefinitionAdapter
  ExtensionManifest->>ExtensibilityService: Declare nativescript.commands map
  ExtensibilityService->>CommandRegistry: Register deferred command
  CommandRegistry->>LazyCommandModule: Load command module on lookup
  LazyCommandModule->>CommandDefinitionAdapter: Adapt exported command definition
  CommandDefinitionAdapter->>CommandRegistry: Register definition under manifest name
Loading

Possibly related PRs

Suggested reviewers: nathanwalker

Poem

I’m a rabbit with commands in a map,
Loading each burrow only when tapped.
Conflicts are checked, aliases align,
Definitions register under each sign.
Lazy hops keep the CLI bright—
Tests guard every route just right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies per-command lazy loading through the nativescript.commands map, which is the primary change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

@edusperoni
edusperoni force-pushed the feat/define-command branch from b15734b to 863f964 Compare July 30, 2026 01:39
@edusperoni
edusperoni force-pushed the feat/extension-manifests branch from 220e027 to 02d6f7c Compare July 30, 2026 01:41
@edusperoni
edusperoni force-pushed the feat/define-command branch from 863f964 to 74caa8f Compare July 30, 2026 01:44
@edusperoni
edusperoni force-pushed the feat/extension-manifests branch from 02d6f7c to d8a8fcf Compare July 30, 2026 01:44
@edusperoni
edusperoni force-pushed the feat/define-command branch from 74caa8f to cafa737 Compare July 30, 2026 02:27
@edusperoni
edusperoni force-pushed the feat/extension-manifests branch from d8a8fcf to e0c671c Compare July 30, 2026 02:28
@edusperoni
edusperoni force-pushed the feat/define-command branch from cafa737 to a1ba0ef Compare July 30, 2026 02:51
@edusperoni
edusperoni force-pushed the feat/extension-manifests branch from e0c671c to 10aaa87 Compare July 30, 2026 02:52
@edusperoni
edusperoni force-pushed the feat/define-command branch from a1ba0ef to 07c979c Compare August 4, 2026 20:41
@edusperoni
edusperoni force-pushed the feat/extension-manifests branch from 10aaa87 to 7bbf81e Compare August 4, 2026 20:41
…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.
…rgument 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.
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.
`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.
@edusperoni
edusperoni force-pushed the feat/define-command branch from 07c979c to d712a4e Compare August 5, 2026 19:22
…s 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.
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.
… 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.
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.
…iner

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.
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.
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.
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.
@edusperoni
edusperoni force-pushed the feat/extension-manifests branch from 7bbf81e to bcd08f0 Compare August 5, 2026 19:44
@edusperoni
edusperoni marked this pull request as ready for review August 5, 2026 20:22
Base automatically changed from feat/define-command to main August 6, 2026 04:09
@NathanWalker

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Co-authored-by: NathanWalker <457187+NathanWalker@users.noreply.github.com>

Copilot AI commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved and pushed in commit 6568c06 by merging origin/main into this branch and fixing the conflicts in defining-commands.md, lib/common/services/command-definition-adapter.ts, and lib/contracts/index.ts.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
test/extension-manifests.ts (1)

592-619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the single module load in the alias test.

The test name states that the shared module is loaded once. The body does not assert that. Add an assertion on capture.loadedModules after both resolutions, so a regression that reloads the module per alias fails this test.

♻️ Proposed assertion
 			assert.isOk(testInjector.resolveCommand("nsmalias|run"));
 			const aliased = testInjector.resolveCommand("nsmalias|r");
 			assert.isOk(aliased);
+			assert.deepEqual(capture.loadedModules, ["alias-run"]);
 
 			await aliased.execute(["x"]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extension-manifests.ts` around lines 592 - 619, Add an assertion to the
alias test after resolving both commands in “routes two aliases of one command
to the same module” that verifies capture.loadedModules contains exactly one
load of the shared module; keep the existing command execution and
capture.executed assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@extensions.md`:
- Line 18: Convert the section headings in extensions.md, including the heading
near “Depending on the CLI” and those at the referenced locations, from ATX
syntax to setext syntax consistent with the file and related documentation. Add
the text language identifier to the unlabeled fenced block near line 263 while
preserving its contents.

In `@lib/common/yok.ts`:
- Around line 159-233: Update registerDeferredCommand to reject a hierarchical
child when its direct parent command is already registered, before calling
super.register or mutating ownership/command state. Add a dedicated structured
reason to DeferredCommandRejection and handle it in describeRejection,
preserving existing behavior for valid parent-child registrations.
- Around line 107-108: Initialize deferredCommandOwners with a null-prototype
object via Object.create(null) instead of a normal object, so command names such
as constructor cannot resolve inherited Object.prototype properties during
ownership checks.

In `@test/extension-manifests.ts`:
- Around line 462-479: Update the fixture generation logic around
definitionModule to resolve the contracts module through Vitest’s requireService
loader before constructing the generated JavaScript, instead of using plain
require.resolve. Ensure the generated fixture embeds the loader-resolved path so
its later plain require can load lib/contracts consistently.

---

Nitpick comments:
In `@test/extension-manifests.ts`:
- Around line 592-619: Add an assertion to the alias test after resolving both
commands in “routes two aliases of one command to the same module” that verifies
capture.loadedModules contains exactly one load of the shared module; keep the
existing command execution and capture.executed assertions unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7b8aebe3-5ac4-44bf-a4d4-08d2aba24c24

📥 Commits

Reviewing files that changed from the base of the PR and between 2f81d7f and 6568c06.

📒 Files selected for processing (13)
  • defining-commands.md
  • dependency-injection.md
  • extensions.md
  • lib/common/contracts/command-registry.ts
  • lib/common/contracts/index.ts
  • lib/common/definitions/extensibility.d.ts
  • lib/common/di/index.ts
  • lib/common/di/injector.ts
  • lib/common/di/providers.ts
  • lib/common/services/command-definition-adapter.ts
  • lib/common/yok.ts
  • lib/services/extensibility-service.ts
  • test/extension-manifests.ts

Comment thread extensions.md
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.

## Depending on the CLI

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Match the repository heading style and label the output fence.

markdownlint reports MD003 for every ## heading in this file. The file itself opens with a setext heading at Lines 1-2, and defining-commands.md and dependency-injection.md use setext throughout. Convert the section headings to setext. Also add a language to the fenced block at Line 263 to clear MD040.

📝 Proposed fixes
-## Depending on the CLI
+Depending on the CLI
+--------------------
-```
+```text
 The command hello world is registered in extension nativescript-hello.
 You can install it by executing 'ns extension install nativescript-hello'

Apply the same setext conversion to the headings at Lines 66, 71, 142, 165, 205, 236, 250, and 268.
</details>


Also applies to: 263-263

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.23.2)</summary>

[warning] 18-18: Heading style
Expected: setext; Actual: atx

(MD003, heading-style)

</details>

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @extensions.md at line 18, Convert the section headings in extensions.md,
including the heading near “Depending on the CLI” and those at the referenced
locations, from ATX syntax to setext syntax consistent with the file and related
documentation. Add the text language identifier to the unlabeled fenced block
near line 263 while preserving its contents.


</details>

<!-- fingerprinting:phantom:medusa:komodo -->

<!-- cr-indicator-types:potential_issue -->

<!-- cr-comment:v1:2480f365076ff0b80a97457f -->

_Source: Linters/SAST tools_

<!-- This is an auto-generated comment by CodeRabbit -->

Comment thread lib/common/yok.ts
Comment on lines +107 to +108
/** Deferred command name -> the owner that claimed it first. */
private deferredCommandOwners: IDictionary<string> = {};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use a null-prototype map for deferred ownership.

A manifest can declare the lowercase command name constructor. Line 172 then reads the inherited Object.prototype.constructor value and returns a false "claimed" rejection. Initialize deferredCommandOwners with Object.create(null).

Proposed fix
- private deferredCommandOwners: IDictionary<string> = {};
+ private deferredCommandOwners: IDictionary<string> = Object.create(null);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/** Deferred command name -> the owner that claimed it first. */
private deferredCommandOwners: IDictionary<string> = {};
/** Deferred command name -> the owner that claimed it first. */
private deferredCommandOwners: IDictionary<string> = Object.create(null);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/common/yok.ts` around lines 107 - 108, Initialize deferredCommandOwners
with a null-prototype object via Object.create(null) instead of a normal object,
so command names such as constructor cannot resolve inherited Object.prototype
properties during ownership checks.

Comment thread lib/common/yok.ts
Comment on lines +159 to +233
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Reject a child command when its direct parent already exists.

If a manifest declares foo before foo|bar, this method registers foo|bar and reports success. Lines 231-233 then call createHierarchicalCommand, which detects the direct foo command, logs that foo|bar cannot be reached, and does not install a dispatcher. Reject this conflict before registering the child. Add a structured rejection reason and update DeferredCommandRejection and describeRejection with the same reason.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/common/yok.ts` around lines 159 - 233, Update registerDeferredCommand to
reject a hierarchical child when its direct parent command is already
registered, before calling super.register or mutating ownership/command state.
Add a dedicated structured reason to DeferredCommandRejection and handle it in
describeRejection, preserving existing behavior for valid parent-child
registrations.

Comment on lines +462 to +479
const contractsPath = require.resolve("../lib/contracts");

const definitionModule = (
commandName: string,
marker: string,
exportAs: string = "module.exports",
): string =>
`const { defineCommand } = require(${JSON.stringify(contractsPath)});
global.__nsmCapture.loadedModules.push(${JSON.stringify(marker)});
${exportAs} = defineCommand({
name: ${JSON.stringify(commandName)},
arguments: "any",
async run(ctx) {
global.__nsmCapture.executed.push({ marker: ${JSON.stringify(
marker,
)}, args: ctx.args });
},
});`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect how `lib/contracts` resolves and how tests are executed.
set -euo pipefail

fd -a 'contracts' lib --max-depth 2
fd -a 'index.ts' lib/contracts 2>/dev/null || true

# Test runner configuration and TS handling
fd -H -t f 'vitest.config.*|vite.config.*|tsconfig*.json' . --max-depth 2 --exec cat -n {}

# How the test script is invoked
rg -n '"(test|pretest|build)"\s*:' package.json -A2

Repository: NativeScript/nativescript-cli

Length of output: 2626


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate extension-manifests.ts and inspect the relevant fixture-generation snippet plus any build hooks.
fd -a 'extension-manifests.ts' test scripts --max-depth 3 --exec sh -c '
  echo "FILE: $1"
  wc -l "$1"
  sed -n "420,530p" "$1" | cat -n
' sh {}

# Check whether lib/contracts has a package.json or otherwise supports Node require from lib-relative paths.
if [ -f lib/contracts/package.json ]; then
  cat -n lib/contracts/package.json
else
  echo "NO_PACKAGE_JSON"
fi

# Show lockfile/package versions related to vitest/vite/ts to infer runtime support if needed.
for f in package-lock.json pnpm-lock.yaml yarn.lock bun.lock; do
  if [ -f "$f" ]; then
    echo "LOCKFILE: $f"
    rg -n "vitest|typescript|ts-node" "$f" -A2 -B2 | head -n 80 || true
  fi
done

# Behavioral probe from repository file contents using Node without running repository scripts:
# - Resolve the same path from the repo root using node's internal require.resolve semantics if available.
# - Check whether the target file exists at the TypeScript source vs package.json "main" vs compiled dist target.
node - <<'JS'
const fs = require('fs');
const path = require('path');

console.log('NODE_VERSION', process.version);
console.log('CWD', process.cwd());

const source = path.join(process.cwd(), 'lib/contracts/index.ts');
const dist = path.join(process.cwd(), 'dist/lib/contracts/index.js');
const pkg = path.join(process.cwd(), 'lib/contracts/package.json');

console.log('SOURCE_EXISTS', fs.existsSync(source));
console.log('SOURCE_REALPATH', fs.realpathSync(source));
console.log('DIST_EXISTS', fs.existsSync(dist));
console.log('PKG_EXISTS', fs.existsSync(pkg));
console.log('PKG_CONTENT', fs.existsSync(pkg) ? JSON.parse(fs.readFileSync(pkg, 'utf8')) : null);

try {
  const r = require.resolve('../lib/contracts');
  console.log('REQUIRE_RESOLVE', r);
  console.log('REQUIRE_RESOLVE_EXISTS', fs.existsSync(r) || fs.existsSync(r + '.js') || fs.existsSync(r + '.' + (path.extname(r) || '').slice(1)));
  console.log('FS_STAT', fs.statSync(r));
} catch (e) {
  console.log('REQUIRE_RESOLVE_ERROR', e.code || e.message);
}
JS

Repository: NativeScript/nativescript-cli

Length of output: 7774


Route requireService through Vitest’s module loader before using require.resolve.

require.resolve("../lib/contracts") resolves inside the test process, but the generated fixture is a compiled .js file that later runs with plain require. Since lib/contracts/index.ts is not a Node module, this can fail unless Vitest’s loader intercepts that load. Make the fixture generation path consistent with test execution.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extension-manifests.ts` around lines 462 - 479, Update the fixture
generation logic around definitionModule to resolve the contracts module through
Vitest’s requireService loader before constructing the generated JavaScript,
instead of using plain require.resolve. Ensure the generated fixture embeds the
loader-resolved path so its later plain require can load lib/contracts
consistently.

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.

3 participants