From 678cf61e04afeace9af25313ce219b665f1e12cd Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 21:57:43 -0300 Subject: [PATCH 1/5] feat(hooks): add defineHook with typed ctx (wrap/abort/payload) Hook authors can now export a definition built with defineHook instead of a plain function whose shape the CLI has to infer. The handler takes a context object with the operation payload, an explicit wrap() for middleware around the hooked method, and abort() for stopping the hook as either a failure or a warning. Definitions are marked with Symbol.for("nativescript:cli:hookDefinition") so a duplicated CLI copy in an extension's dependency tree still recognizes them. The definition path skips parameter-name resolution, the projectData promotion hack and the deprecation report; plain function hooks keep running through the existing path unchanged. lib/common/define-hook.ts is import-free so a hook can load it without booting a second runtime, and it is re-exported from nativescript/contracts. --- extending-cli.md | 61 ++++- lib/common/define-hook.ts | 111 +++++++++ lib/common/services/hooks-service.ts | 186 +++++++++------ lib/contracts/index.ts | 8 + test/define-hook.ts | 327 +++++++++++++++++++++++++++ 5 files changed, 621 insertions(+), 72 deletions(-) create mode 100644 lib/common/define-hook.ts create mode 100644 test/define-hook.ts diff --git a/extending-cli.md b/extending-cli.md index 5ace718f18..fec73320ca 100644 --- a/extending-cli.md +++ b/extending-cli.md @@ -77,11 +77,58 @@ Execute Hooks In-Process When your hook is a Node.js script, the CLI executes it in-process. This gives you access to the entire internal state of the CLI and all of its functions. -The CLI assumes that this is a CommonJS module and calls its single exported function. +The CLI assumes that this is a CommonJS module and calls the hook it exports — either a hook definition (see below) or a plain function. ## Writing a hook -Hooks run inside an injection context, so services come from `inject()` — the same API used everywhere else (see [dependency-injection.md](dependency-injection.md)). Declare a `hookArgs` parameter only if you need the payload of the operation being hooked. +Export a hook definition built with `defineHook`. It takes the hook point in the usual naming convention (`before-prepare`, `after-watch`) and a handler that receives a context object. + +```JavaScript +const { defineHook, inject, DoctorService } = require("nativescript/contracts"); + +module.exports = defineHook("before-prepare", async (ctx) => { + const doctorService = inject(DoctorService); + await doctorService.canExecuteLocalBuild(); +}); +``` + +Services come from `inject()` — the same API used everywhere else (see [dependency-injection.md](dependency-injection.md)): + +* `inject()` is valid in the synchronous part of the handler — not after an `await`. Resolve what you need up front; for late lookups, grab the container first: `const injector = inject(Injector)` (`Injector` is exported from `nativescript/contracts` too), then `injector.get(...)` later. +* Tokens resolve by class first and by their canonical name on a miss, so this works even if your dependency tree carries its own copy of `nativescript` — a duplicated token class still resolves to the running CLI's service. +* Only a first tranche of services has typed tokens so far ([dependency-injection.md](dependency-injection.md#available-contracts) lists them); a service without a token is reachable by its registry name — `inject("logger")` — as a migration bridge. +* If you build your hook in TypeScript, add `nativescript` as a `devDependency` and import the same names: `import { defineHook, inject, DoctorService } from "nativescript/contracts"`. An `.mjs` hook can `export default defineHook(...)`. + +`ctx.payload` holds the parameters of the CLI operation being hooked; its shape depends on the hook point. It is the CLI's own object, so mutating it influences the operation: + +```JavaScript +module.exports = defineHook("before-build-task-args", (ctx) => { + ctx.payload.args.push("--offline"); +}); +``` + +`ctx.wrap(middleware)` puts a middleware around the hooked method. The middleware receives the method's arguments and a `next` callback; call `next` to continue, or return without calling it to short-circuit the method entirely. Register it from a `before-` hook. + +```JavaScript +module.exports = defineHook("before-prepare", (ctx) => { + ctx.wrap(async (args, next) => { + const result = await next(...args); + return result; + }); +}); +``` + +`ctx.abort(message)` stops the hook and fails the command. Pass `{ asWarning: true }` to print the message as a warning and let the command continue instead. + +```JavaScript +module.exports = defineHook("before-prepare", (ctx) => { + ctx.abort("Nothing to prepare.", { asWarning: true }); +}); +``` + +### Plain function hooks + +Exporting a plain function is still supported. It runs in an injection context too, so `inject()` works the same way; declare a `hookArgs` parameter if you need the payload. ```JavaScript const { inject, DoctorService } = require("nativescript/contracts"); @@ -92,12 +139,6 @@ module.exports = function (hookArgs) { }; ``` -* `inject()` is valid in the synchronous part of the hook body — not after an `await`. Resolve what you need up front; for late lookups, grab the container first: `const injector = inject(Injector)` (`Injector` is exported from `nativescript/contracts` too), then `injector.get(...)` later. -* `hookArgs` contains the parameters of the CLI function being hooked; its shape depends on the hook point. Declare it only when you need it — a hook may also take no parameters at all. A future typed hook API (`defineHook` with an explicit context object) will replace this parameter; it is the one remaining piece of the legacy convention. -* Tokens resolve by class first and by their canonical name on a miss, so this works even if your dependency tree carries its own copy of `nativescript` — a duplicated token class still resolves to the running CLI's service. -* Only a first tranche of services has typed tokens so far ([dependency-injection.md](dependency-injection.md#available-contracts) lists them); a service without a token is reachable by its registry name — `inject("logger")` — as a migration bridge. -* If you build your hook in TypeScript, add `nativescript` as a `devDependency` and import the same names: `import { inject, DoctorService } from "nativescript/contracts"`. - ## The hook contract The hook must return a Promise. If the hook succeeds, it must fullfil the promise, but the fullfilment value is ignored. @@ -110,6 +151,10 @@ Member | Type | Description If these two members are not set, the CLI prints the returned error colored as fatal error and stops executing the current command. +A plain-function hook can also return a function, which the CLI folds into a middleware chain around the hooked method. + +With `defineHook` neither convention is needed: `ctx.abort` replaces throwing an error carrying `stopExecution`/`errorAsWarning`, and `ctx.wrap` replaces returning a function. + ## Legacy: parameter-name injection Historically, a hook received CLI services by naming them as parameters: the CLI parses the exported function's parameter names and injects the service registered under each name. Existing hooks written this way keep working unchanged, but **new hooks should use the pattern above** — parameter-name service injection is slated for deprecation, and hooks that use it are reported through the CLI's deprecation tracer (visible with `--log trace`, or as warnings with `NS_DEPRECATIONS=warn`). diff --git a/lib/common/define-hook.ts b/lib/common/define-hook.ts new file mode 100644 index 0000000000..e497cf6663 --- /dev/null +++ b/lib/common/define-hook.ts @@ -0,0 +1,111 @@ +/** + * The typed hook-authoring API. Kept import-free so that a hook (or an + * extension carrying its own copy of the CLI) can load it without booting a + * second runtime — importing lib/common/yok creates global.$injector. + */ + +/** + * `Symbol.for` rather than a module-local symbol: an extension may resolve a + * duplicated copy of the CLI from its own node_modules, and the running CLI + * still has to recognize definitions minted by that copy. + */ +export const HOOK_DEFINITION_MARKER = Symbol.for( + "nativescript:cli:hookDefinition", +); + +/** + * Wraps the method the hook point decorates. `next` continues the chain — call + * it with `args` to run the original, or skip it to short-circuit. + */ +export type HookMiddleware = ( + args: any[], + next: (...args: any[]) => any, +) => any; + +export interface IHookContext { + /** + * The payload of the operation being hooked. Its shape depends on the hook + * point, and it is the caller's own object: mutating it is a supported + * channel for influencing the operation. + */ + payload: any; + + /** Registers a middleware around the method this hook point decorates. */ + wrap(middleware: HookMiddleware): void; + + /** + * Stops the hook. With `asWarning`, the CLI logs the message and continues + * the command; otherwise the command fails. + */ + abort(message: string, opts?: { asWarning?: boolean }): never; +} + +export type HookHandler = (ctx: IHookContext) => void | Promise; + +export interface IHookDefinition { + /** Hook point, in the hyphen convention: `before-prepare`, `after-watch`. */ + readonly name: string; + readonly handler: HookHandler; +} + +export interface IHookInvocation { + context: IHookContext; + /** Populated by `ctx.wrap()` while the handler runs. */ + middlewares: HookMiddleware[]; +} + +export function defineHook( + name: string, + handler: HookHandler, +): IHookDefinition { + const definition: IHookDefinition = { name, handler }; + Object.defineProperty(definition, HOOK_DEFINITION_MARKER, { value: true }); + return definition; +} + +export function isHookDefinition(value: any): boolean { + return ( + !!value && + (typeof value === "object" || typeof value === "function") && + value[HOOK_DEFINITION_MARKER] === true && + typeof value.handler === "function" + ); +} + +/** + * Derives the context from the raw hook argument bag: the `hookArgs` wrapper + * when the hook point supplies one, the bag itself for hook points that pass + * their keys at the top level, and nothing when there is no payload. + */ +export function createHookInvocation(hookArguments: any): IHookInvocation { + const middlewares: HookMiddleware[] = []; + const context: IHookContext = { + payload: derivePayload(hookArguments), + wrap(middleware: HookMiddleware): void { + middlewares.push(middleware); + }, + abort(message: string, opts?: { asWarning?: boolean }): never { + const error: any = new Error(message); + if (opts && opts.asWarning) { + // The pair the hooks service checks for to downgrade a rejection. + error.stopExecution = false; + error.errorAsWarning = true; + } + throw error; + }, + }; + + return { context, middlewares }; +} + +function derivePayload(hookArguments: any): any { + if (!hookArguments || typeof hookArguments !== "object") { + return undefined; + } + + if ("hookArgs" in hookArguments) { + return hookArguments["hookArgs"]; + } + + return Object.keys(hookArguments).length ? hookArguments : undefined; +} diff --git a/lib/common/services/hooks-service.ts b/lib/common/services/hooks-service.ts index b1638ede0c..b3265e9f12 100644 --- a/lib/common/services/hooks-service.ts +++ b/lib/common/services/hooks-service.ts @@ -3,6 +3,9 @@ import * as util from "util"; import * as _ from "lodash"; import { annotate, getValueFromNestedObject } from "../helpers"; import { reportDeprecation } from "../deprecation"; +import { createHookInvocation, isHookDefinition } from "../define-hook"; +import type { HookMiddleware, IHookDefinition } from "../define-hook"; +import { runInInjectionContext } from "../di/inject"; import { AnalyticsEventLabelDelimiter } from "../../constants"; import { IOptions, IPerformanceService } from "../../declarations"; import { @@ -221,80 +224,96 @@ export class HooksService implements IHooksService { : hookModule; } - if (typeof hookEntryPoint !== "function") { + // Covers both a `.mjs` default export and tsc's CommonJS emit of + // `export default`, whose value lands under `.default`. + const definitionCandidate = + (hookEntryPoint && hookEntryPoint.default) ?? hookEntryPoint; + + if (isHookDefinition(definitionCandidate)) { + result = await this.executeHookDefinition( + definitionCandidate, + hookName, + hook, + hookArguments, + ); + } else if (typeof hookEntryPoint !== "function") { + // A definition is a plain object, so this guard has to stay below the + // definition check. this.$logger.warn( `${hook.fullPath} will NOT be executed because it does not export a function.`, ); return; - } - - this.$logger.trace(`Validating ${hookName} arguments.`); - - const invalidArguments = this.validateHookArguments( - hookEntryPoint, - hook.fullPath, - ); + } else { + this.$logger.trace(`Validating ${hookName} arguments.`); - if (invalidArguments.length) { - this.$logger.warn( - `${ - hook.fullPath - } will NOT be executed because it has invalid arguments - ${color.grey( - invalidArguments.join(", "), - )}.`, + const invalidArguments = this.validateHookArguments( + hookEntryPoint, + hook.fullPath, ); - return; - } - // HACK for backwards compatibility: - // In case $projectData wasn't resolved by the time we got here (most likely we got here without running a command but through a service directly) - // then it is probably passed as a hookArg - // if that is the case then pass it directly to the hook instead of trying to resolve $projectData via injector - // This helps make hooks stateless - const projectDataHookArg = - hookArguments["hookArgs"] && hookArguments["hookArgs"]["projectData"]; - if (projectDataHookArg) { - hookArguments["projectData"] = hookArguments["$projectData"] = - projectDataHookArg; - } + if (invalidArguments.length) { + this.$logger.warn( + `${ + hook.fullPath + } will NOT be executed because it has invalid arguments - ${color.grey( + invalidArguments.join(", "), + )}.`, + ); + return; + } - // Only param-name *service* injection is on the deprecation track; a - // hook declaring nothing but `hookArgs` (or no parameters) already - // follows the recommended pattern and must not be flagged. - const usesParamNameInjection = (( - hookEntryPoint.$inject.args - )).some((argument) => argument !== this.hookArgsName); - if (usesParamNameInjection) { - reportDeprecation({ - api: "hooks.param-name-signature", - detail: hook.fullPath, - logger: this.$logger, - }); - } + // HACK for backwards compatibility: + // In case $projectData wasn't resolved by the time we got here (most likely we got here without running a command but through a service directly) + // then it is probably passed as a hookArg + // if that is the case then pass it directly to the hook instead of trying to resolve $projectData via injector + // This helps make hooks stateless + const projectDataHookArg = + hookArguments["hookArgs"] && hookArguments["hookArgs"]["projectData"]; + if (projectDataHookArg) { + hookArguments["projectData"] = hookArguments["$projectData"] = + projectDataHookArg; + } - const maybePromise = this.$injector.resolve( - hookEntryPoint, - hookArguments, - ); - if (maybePromise) { - this.$logger.trace("Hook promises to signal completion"); - try { - result = await maybePromise; - } catch (err) { - if ( - err && - _.isBoolean(err.stopExecution) && - err.errorAsWarning === true - ) { - this.$logger.warn(err.message || err); - } else { - // Print the actual error with its callstack, so it is easy to find out which hooks is causing troubles. - this.$logger.error(err); - throw err || new Error(`Failed to execute hook: ${hook.fullPath}.`); - } + // Only param-name *service* injection is on the deprecation track; a + // hook declaring nothing but `hookArgs` (or no parameters) already + // follows the recommended pattern and must not be flagged. + const usesParamNameInjection = (( + hookEntryPoint.$inject.args + )).some((argument) => argument !== this.hookArgsName); + if (usesParamNameInjection) { + reportDeprecation({ + api: "hooks.param-name-signature", + detail: hook.fullPath, + logger: this.$logger, + }); } - this.$logger.trace("Hook completed"); + const maybePromise = this.$injector.resolve( + hookEntryPoint, + hookArguments, + ); + if (maybePromise) { + this.$logger.trace("Hook promises to signal completion"); + try { + result = await maybePromise; + } catch (err) { + if ( + err && + _.isBoolean(err.stopExecution) && + err.errorAsWarning === true + ) { + this.$logger.warn(err.message || err); + } else { + // Print the actual error with its callstack, so it is easy to find out which hooks is causing troubles. + this.$logger.error(err); + throw ( + err || new Error(`Failed to execute hook: ${hook.fullPath}.`) + ); + } + } + + this.$logger.trace("Hook completed"); + } } } else { const environment = this.prepareEnvironment(hook.fullPath); @@ -333,6 +352,42 @@ export class HooksService implements IHooksService { return result; } + private async executeHookDefinition( + definition: IHookDefinition, + hookName: string, + hook: IHook, + hookArguments: IDictionary, + ): Promise { + if (definition.name !== hookName) { + this.$logger.trace( + `Hook ${hook.fullPath} defines "${definition.name}" but its file name places it at the "${hookName}" hook point; running it there.`, + ); + } + + const { context, middlewares } = createHookInvocation(hookArguments); + const container = this.$injector.di; + + try { + await runInInjectionContext(container, () => definition.handler(context)); + } catch (err) { + if ( + err && + _.isBoolean(err.stopExecution) && + err.errorAsWarning === true + ) { + this.$logger.warn(err.message || err); + } else { + // Print the actual error with its callstack, so it is easy to find out which hooks is causing troubles. + this.$logger.error(err); + throw err || new Error(`Failed to execute hook: ${hook.fullPath}.`); + } + } + + this.$logger.trace("Hook completed"); + + return middlewares.length ? middlewares : undefined; + } + private async executeHooksInDirectory( directoryPath: string, hookName: string, @@ -356,7 +411,10 @@ export class HooksService implements IHooksService { } } - return results; + // executeHooks flattens the per-directory results exactly once, so a hook + // returning several middlewares must contribute them individually or they + // stay nested one level too deep for decorateMethod's function filter. + return _.flatten(results); } private getCustomHooksByName(hookName: string): IHook[] { diff --git a/lib/contracts/index.ts b/lib/contracts/index.ts index c13a9b9996..c65b022042 100644 --- a/lib/contracts/index.ts +++ b/lib/contracts/index.ts @@ -25,3 +25,11 @@ export type { export { DoctorService } from "./doctor-service"; export { ProjectNameService } from "./project-name-service"; + +export { defineHook, isHookDefinition } from "../common/define-hook"; +export type { + IHookContext, + IHookDefinition, + HookHandler, + HookMiddleware, +} from "../common/define-hook"; diff --git a/test/define-hook.ts b/test/define-hook.ts new file mode 100644 index 0000000000..be660859bd --- /dev/null +++ b/test/define-hook.ts @@ -0,0 +1,327 @@ +import { assert } from "chai"; +import * as fs from "fs"; +import * as os from "os"; +import * as path from "path"; +import { Yok } from "../lib/common/yok"; +import { HooksService } from "../lib/common/services/hooks-service"; +import { hook } from "../lib/common/helpers"; +import { IInjector } from "../lib/common/definitions/yok"; +import { IHooksService } from "../lib/common/declarations"; +import { LoggerStub, ErrorsStub } from "./stubs"; + +// Hook fixtures load the API the way a real hook does — through the published +// `nativescript/contracts` entry point — so the marker symbol, the context +// shape and the hooks-service integration are exercised end to end. +const apiPath = require.resolve("../lib/contracts"); + +function createTestInjector(projectDir: string): IInjector { + const testInjector = new Yok(); + testInjector.register("logger", LoggerStub); + testInjector.register("errors", ErrorsStub); + testInjector.register("fs", { + exists: (p: string) => fs.existsSync(p), + getFsStats: (p: string) => fs.statSync(p), + readDirectory: (p: string) => fs.readdirSync(p), + readText: (p: string) => fs.readFileSync(p, "utf8"), + }); + testInjector.register("childProcess", {}); + testInjector.register("config", { DISABLE_HOOKS: false }); + testInjector.register("staticConfig", { + CLIENT_NAME: "tns", + version: "0.0.0", + }); + testInjector.register("projectHelper", { projectDir }); + testInjector.register("options", { hooks: true }); + testInjector.register("performanceService", { + now: () => 0, + processExecutionData: () => { + /* not measured here */ + }, + }); + testInjector.register("projectConfigService", { + getValue: (_key: string, defaultValue: any) => defaultValue, + }); + testInjector.register("projectData", { fromContainer: true }); + testInjector.register("hooksService", HooksService); + return testInjector; +} + +function writeHook( + projectDir: string, + hookName: string, + source: string, + extension = ".js", +): string { + const hooksDir = path.join(projectDir, "hooks"); + fs.mkdirSync(hooksDir, { recursive: true }); + const fullPath = path.join(hooksDir, `${hookName}${extension}`); + fs.writeFileSync(fullPath, source); + return fullPath; +} + +function writeHookInDirectory( + projectDir: string, + hookName: string, + fileName: string, + source: string, +): string { + const hooksDir = path.join(projectDir, "hooks", hookName); + fs.mkdirSync(hooksDir, { recursive: true }); + const fullPath = path.join(hooksDir, fileName); + fs.writeFileSync(fullPath, source); + return fullPath; +} + +describe("defineHook", () => { + let projectDir: string; + let testInjector: IInjector; + let capture: any; + + const hooksService = (): IHooksService => + testInjector.resolve("hooksService"); + const logger = (): LoggerStub => testInjector.resolve("logger"); + + beforeEach(() => { + projectDir = fs.mkdtempSync(path.join(os.tmpdir(), "ns-define-hook-")); + testInjector = createTestInjector(projectDir); + capture = (global).__hookCapture = {}; + }); + + afterEach(() => { + fs.rmSync(projectDir, { recursive: true, force: true }); + delete (global).__hookCapture; + }); + + it("passes the hookArgs value as the payload, by identity and mutable in place", async () => { + writeHook( + projectDir, + "before-case1", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case1", async (ctx) => { + global.__hookCapture.payload = ctx.payload; + ctx.payload.args.push("--offline"); + });`, + ); + + const args = ["assembleDebug"]; + const payload = { args }; + await hooksService().executeBeforeHooks("case1", { hookArgs: payload }); + + assert.strictEqual(capture.payload, payload); + assert.deepEqual(args, ["assembleDebug", "--offline"]); + }); + + it("passes the top-level bag as the payload when there is no hookArgs wrapper", async () => { + writeHook( + projectDir, + "after-case2", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("after-case2", (ctx) => { + global.__hookCapture.payload = ctx.payload; + });`, + ); + + const liveSyncResultInfo = { fake: true }; + await hooksService().executeAfterHooks("case2", { liveSyncResultInfo }); + + assert.strictEqual(capture.payload.liveSyncResultInfo, liveSyncResultInfo); + }); + + it("leaves the payload undefined for a hook point with no arguments", async () => { + writeHook( + projectDir, + "before-case3", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case3", (ctx) => { + global.__hookCapture.ran = true; + global.__hookCapture.payload = ctx.payload; + });`, + ); + + await hooksService().executeBeforeHooks("case3"); + + assert.isTrue(capture.ran); + assert.isUndefined(capture.payload); + }); + + it("runs the handler in an injection context, so inject() resolves by token and by name", async () => { + writeHook( + projectDir, + "before-case4", + `const { defineHook, inject, Injector } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case4", async (ctx) => { + global.__hookCapture.container = inject(Injector); + global.__hookCapture.logger = inject("logger"); + });`, + ); + + await hooksService().executeBeforeHooks("case4"); + + assert.strictEqual(capture.container, (testInjector).di); + assert.strictEqual(capture.logger, testInjector.resolve("logger")); + }); + + it("folds a wrap() middleware into the chain around the @hook-decorated method", async () => { + writeHook( + projectDir, + "before-case5", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case5", (ctx) => { + ctx.wrap(function (args, next) { + global.__hookCapture.middlewareArgs = args.slice(); + return next.apply(null, args).then(function (result) { + return "wrapped(" + result + ")"; + }); + }); + });`, + ); + + class Subject { + constructor(public $hooksService: IHooksService) {} + + @hook("case5") + async doWork(input: string): Promise { + (global).__hookCapture.originalRan = true; + return "original:" + input; + } + } + + const subject = testInjector.resolve(Subject); + const result = await subject.doWork("x"); + + assert.equal(result, "wrapped(original:x)"); + assert.isTrue(capture.originalRan); + assert.deepEqual(capture.middlewareArgs, ["x"]); + }); + + it("lets a wrap() middleware short-circuit the decorated method", async () => { + writeHook( + projectDir, + "before-case6", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case6", (ctx) => { + ctx.wrap(function () { + return "short-circuited"; + }); + });`, + ); + + class Subject { + constructor(public $hooksService: IHooksService) {} + + @hook("case6") + async doWork(): Promise { + (global).__hookCapture.originalRan = true; + return "original"; + } + } + + const subject = testInjector.resolve(Subject); + const result = await subject.doWork(); + + assert.equal(result, "short-circuited"); + assert.isUndefined(capture.originalRan); + }); + + it("logs a warning and continues when the handler aborts with asWarning", async () => { + writeHook( + projectDir, + "before-case7", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case7", async (ctx) => { + ctx.abort("soft-abort", { asWarning: true }); + });`, + ); + + await hooksService().executeBeforeHooks("case7"); + + assert.include(logger().warnOutput, "soft-abort"); + }); + + it("fails the command when the handler aborts without asWarning", async () => { + writeHook( + projectDir, + "before-case8", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case8", async (ctx) => { + ctx.abort("hard-abort"); + });`, + ); + + await assert.isRejected( + hooksService().executeBeforeHooks("case8"), + /hard-abort/, + ); + }); + + it("keeps a legacy param-name hook on the old path, and never reports a definition hook", async () => { + const legacyPath = writeHookInDirectory( + projectDir, + "before-case9", + "legacy.js", + `module.exports = function ($logger) { + global.__hookCapture.legacyRan = true; + };`, + ); + const definitionPath = writeHookInDirectory( + projectDir, + "before-case9", + "modern.js", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case9", () => { + global.__hookCapture.definitionRan = true; + });`, + ); + + await hooksService().executeBeforeHooks("case9"); + + assert.isTrue(capture.legacyRan); + assert.isTrue(capture.definitionRan); + + const deprecationReports = logger() + .traceOutput.split("\n") + .filter((line) => line.indexOf("hooks.param-name-signature") !== -1); + assert.isTrue( + deprecationReports.some((line) => line.indexOf(legacyPath) !== -1), + ); + assert.isFalse( + deprecationReports.some((line) => line.indexOf(definitionPath) !== -1), + ); + }); + + it("recognizes a definition default-exported from an .mjs hook", async () => { + writeHook( + projectDir, + "before-case10", + `import { createRequire } from "module"; + const require = createRequire(import.meta.url); + const { defineHook } = require(${JSON.stringify(apiPath)}); + export default defineHook("before-case10", (ctx) => { + global.__hookCapture.payload = ctx.payload; + });`, + ".mjs", + ); + + const payload = { fromMjs: true }; + await hooksService().executeBeforeHooks("case10", { hookArgs: payload }); + + assert.strictEqual(capture.payload, payload); + }); + + it("runs a definition whose name differs from the hook point and traces the mismatch", async () => { + writeHook( + projectDir, + "before-case11", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-something-else", () => { + global.__hookCapture.ran = true; + });`, + ); + + await hooksService().executeBeforeHooks("case11"); + + assert.isTrue(capture.ran); + assert.include(logger().traceOutput, `defines "before-something-else"`); + assert.include(logger().traceOutput, `"before-case11" hook point`); + }); +}); From 0e9d1b66fc852287954a0dc80e4709c54cf40a82 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 29 Jul 2026 23:25:56 -0300 Subject: [PATCH 2/5] refactor(hooks): the facade is the injection context Yok extends Injector on the base branch, so definitions run in runInInjectionContext(this.$injector, ...) directly and inject(Injector) inside a hook returns the facade itself. --- lib/common/services/hooks-service.ts | 5 +++-- test/define-hook.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/common/services/hooks-service.ts b/lib/common/services/hooks-service.ts index b3265e9f12..e13ae1bfe6 100644 --- a/lib/common/services/hooks-service.ts +++ b/lib/common/services/hooks-service.ts @@ -365,10 +365,11 @@ export class HooksService implements IHooksService { } const { context, middlewares } = createHookInvocation(hookArguments); - const container = this.$injector.di; try { - await runInInjectionContext(container, () => definition.handler(context)); + await runInInjectionContext(this.$injector, () => + definition.handler(context), + ); } catch (err) { if ( err && diff --git a/test/define-hook.ts b/test/define-hook.ts index be660859bd..d3030f6b96 100644 --- a/test/define-hook.ts +++ b/test/define-hook.ts @@ -157,7 +157,7 @@ describe("defineHook", () => { await hooksService().executeBeforeHooks("case4"); - assert.strictEqual(capture.container, (testInjector).di); + assert.strictEqual(capture.container, testInjector); assert.strictEqual(capture.logger, testInjector.resolve("logger")); }); From 271fd8fdee532184268439ff047ad26926c90512 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 15:36:27 -0300 Subject: [PATCH 3/5] fix(hooks): make the defineHook surface validate and refuse silent no-ops Drops the `I` prefix from the new hook types, makes `run` the handler field with `defineHook({ name, run })` canonical and the positional call kept as sugar, and validates the definition at define time: a missing or non-string name, a non-function run, and unknown fields all throw naming the definition and both accepted forms. The definition marker moves from a non-enumerable defineProperty to a plain assignment so a spread-derived definition stays recognizable, and `isHookDefinition` becomes a type predicate. Behaviors that used to fail quietly now say so: - `ctx.wrap()` only ever ran at the `@hook`-decorated before-points and was dropped everywhere else. Call sites now declare whether they consume middlewares, and `wrap()` throws elsewhere instead. - a definition whose name disagrees with its hook point is skipped with a warning rather than run at a point it was not written for. - `ctx.abort()` with no message produced `Error(undefined)`; it now falls back to a message naming the hook point. - a definition whose run returns a function warns, since the legacy returned-middleware convention does not apply to definitions. - an array export is rejected naming the file, reserving the form for a possible multi-definition module later. `defineHook` / `HookContext` type the payload, as `TPayload | undefined` because dispatch-fired hooks carry none, and executeBeforeHooks is typed with the middleware array it already returns. --- lib/common/declarations.d.ts | 13 +- lib/common/define-hook.ts | 167 ++++++++++++++++++++++--- lib/common/helpers.ts | 1 + lib/common/services/hooks-service.ts | 62 ++++++++-- lib/common/test/unit-tests/stubs.ts | 20 +-- lib/contracts/index.ts | 5 +- test/define-hook.ts | 174 ++++++++++++++++++++++++++- test/stubs.ts | 20 +-- 8 files changed, 399 insertions(+), 63 deletions(-) diff --git a/lib/common/declarations.d.ts b/lib/common/declarations.d.ts index 7051536343..8100843aca 100644 --- a/lib/common/declarations.d.ts +++ b/lib/common/declarations.d.ts @@ -824,12 +824,23 @@ interface IAutoCompletionService { isObsoleteAutoCompletionEnabled(): boolean; } +interface IHookExecutionOptions { + /** + * Set by call sites that fold the returned middlewares around a method (the + * `@hook` decorator). Where nothing consumes them, `ctx.wrap()` rejects + * instead of registering a middleware that would never run. + */ + consumesMiddlewares?: boolean; +} + interface IHooksService { hookArgsName: string; + /** Resolves with the middlewares hooks registered through `ctx.wrap()`. */ executeBeforeHooks( commandName: string, hookArguments?: IDictionary, - ): Promise; + options?: IHookExecutionOptions, + ): Promise; executeAfterHooks( commandName: string, hookArguments?: IDictionary, diff --git a/lib/common/define-hook.ts b/lib/common/define-hook.ts index e497cf6663..1da83c94bb 100644 --- a/lib/common/define-hook.ts +++ b/lib/common/define-hook.ts @@ -8,6 +8,9 @@ * `Symbol.for` rather than a module-local symbol: an extension may resolve a * duplicated copy of the CLI from its own node_modules, and the running CLI * still has to recognize definitions minted by that copy. + * + * Assigned as a plain enumerable property so that `{ ...definition }` keeps the + * marker; symbols stay invisible to Object.keys/for..in/JSON either way. */ export const HOOK_DEFINITION_MARKER = Symbol.for( "nativescript:cli:hookDefinition", @@ -22,15 +25,20 @@ export type HookMiddleware = ( next: (...args: any[]) => any, ) => any; -export interface IHookContext { +export interface HookContext { /** * The payload of the operation being hooked. Its shape depends on the hook * point, and it is the caller's own object: mutating it is a supported - * channel for influencing the operation. + * channel for influencing the operation. Hook points fired by command + * dispatch carry no payload at all, hence `undefined`. */ - payload: any; + payload: TPayload | undefined; - /** Registers a middleware around the method this hook point decorates. */ + /** + * Registers a middleware around the method this hook point decorates. + * Available only to before-hooks of the hook points that fold middlewares + * around a method; elsewhere it throws rather than dropping the middleware. + */ wrap(middleware: HookMiddleware): void; /** @@ -40,52 +48,173 @@ export interface IHookContext { abort(message: string, opts?: { asWarning?: boolean }): never; } -export type HookHandler = (ctx: IHookContext) => void | Promise; +export type HookHandler = ( + ctx: HookContext, +) => void | Promise; + +/** The object bag accepted by `defineHook`. */ +export interface HookDefinitionInput { + /** Hook point, in the hyphen convention: `before-prepare`, `after-watch`. */ + name: string; + run: HookHandler; +} -export interface IHookDefinition { +export interface HookDefinition { /** Hook point, in the hyphen convention: `before-prepare`, `after-watch`. */ readonly name: string; - readonly handler: HookHandler; + readonly run: HookHandler; } -export interface IHookInvocation { - context: IHookContext; +export interface HookInvocation { + context: HookContext; /** Populated by `ctx.wrap()` while the handler runs. */ middlewares: HookMiddleware[]; } -export function defineHook( +const DEFINITION_FIELDS = ["name", "run"]; + +const ACCEPTED_FORMS = + 'defineHook({ name: "before-prepare", run: (ctx) => {} }) or ' + + 'defineHook("before-prepare", (ctx) => {})'; + +function describeDefinition(name: any): string { + return typeof name === "string" && name.length + ? JSON.stringify(name) + : ""; +} + +function failToDefine(message: string): never { + throw new Error(`${message} Accepted forms: ${ACCEPTED_FORMS}.`); +} + +export function defineHook( + definition: HookDefinitionInput, +): HookDefinition; +export function defineHook( name: string, - handler: HookHandler, -): IHookDefinition { - const definition: IHookDefinition = { name, handler }; - Object.defineProperty(definition, HOOK_DEFINITION_MARKER, { value: true }); + run: HookHandler, +): HookDefinition; +export function defineHook( + nameOrDefinition: string | HookDefinitionInput, + run?: HookHandler, +): HookDefinition { + const input = normalizeDefinitionInput(nameOrDefinition, run); + const definition: any = { name: input.name, run: input.run }; + definition[HOOK_DEFINITION_MARKER] = true; + return definition; } -export function isHookDefinition(value: any): boolean { +function normalizeDefinitionInput( + nameOrDefinition: string | HookDefinitionInput, + run?: HookHandler, +): HookDefinitionInput { + if (typeof nameOrDefinition === "string") { + if (!nameOrDefinition.length) { + failToDefine("defineHook() requires a non-empty hook point name."); + } + + if (typeof run !== "function") { + failToDefine( + `defineHook(${describeDefinition(nameOrDefinition)}) requires a handler function as its second argument.`, + ); + } + + return { name: nameOrDefinition, run }; + } + + if ( + !nameOrDefinition || + typeof nameOrDefinition !== "object" || + Array.isArray(nameOrDefinition) + ) { + failToDefine("defineHook() was called with an unsupported argument."); + } + + const unknownFields = Object.keys(nameOrDefinition).filter( + (field) => DEFINITION_FIELDS.indexOf(field) === -1, + ); + if (unknownFields.length) { + failToDefine( + `defineHook(${describeDefinition(nameOrDefinition.name)}) received unknown ` + + `field${unknownFields.length > 1 ? "s" : ""} ` + + `${unknownFields.map((field) => JSON.stringify(field)).join(", ")}. ` + + `Supported fields: ${DEFINITION_FIELDS.map((field) => JSON.stringify(field)).join(", ")}.`, + ); + } + + if (typeof nameOrDefinition.name !== "string" || !nameOrDefinition.name) { + failToDefine( + 'defineHook() requires a non-empty "name" naming the hook point.', + ); + } + + if (typeof nameOrDefinition.run !== "function") { + failToDefine( + `defineHook(${describeDefinition(nameOrDefinition.name)}) requires "run" to be a function.`, + ); + } + + return { name: nameOrDefinition.name, run: nameOrDefinition.run }; +} + +export function isHookDefinition( + value: any, +): value is HookDefinition { return ( !!value && (typeof value === "object" || typeof value === "function") && value[HOOK_DEFINITION_MARKER] === true && - typeof value.handler === "function" + typeof value.run === "function" && + typeof value.name === "string" ); } +export interface HookInvocationOptions { + /** The hook point the definition runs at; used in diagnostics. */ + hookName: string; + /** + * Whether the caller folds the collected middlewares around a method. Only + * the `@hook`-decorated before-points do; everywhere else `ctx.wrap()` has + * nothing to wrap and says so instead of silently dropping the middleware. + */ + consumesMiddlewares?: boolean; +} + /** * Derives the context from the raw hook argument bag: the `hookArgs` wrapper * when the hook point supplies one, the bag itself for hook points that pass * their keys at the top level, and nothing when there is no payload. */ -export function createHookInvocation(hookArguments: any): IHookInvocation { +export function createHookInvocation( + hookArguments: any, + options: HookInvocationOptions, +): HookInvocation { + const { hookName, consumesMiddlewares } = options; const middlewares: HookMiddleware[] = []; - const context: IHookContext = { + const context: HookContext = { payload: derivePayload(hookArguments), wrap(middleware: HookMiddleware): void { + if (!consumesMiddlewares) { + throw new Error( + `ctx.wrap() is not available at the "${hookName}" hook point: nothing folds the middleware around a method there, so it would never run.`, + ); + } + + if (typeof middleware !== "function") { + throw new Error( + `ctx.wrap() expects a function at the "${hookName}" hook point.`, + ); + } + middlewares.push(middleware); }, abort(message: string, opts?: { asWarning?: boolean }): never { - const error: any = new Error(message); + const text = + typeof message === "string" && message.trim().length + ? message + : `The "${hookName}" hook aborted without a message.`; + const error: any = new Error(text); if (opts && opts.asWarning) { // The pair the hooks service checks for to downgrade a rejection. error.stopExecution = false; diff --git a/lib/common/helpers.ts b/lib/common/helpers.ts index 35222fc639..6cdec0c478 100644 --- a/lib/common/helpers.ts +++ b/lib/common/helpers.ts @@ -615,6 +615,7 @@ export function hook(commandName: string) { return hooksService.executeBeforeHooks( commandName, prepareArguments(method, args, hooksService), + { consumesMiddlewares: true }, ); }, async (method: any, self: any, resultPromise: any, args: any[]) => { diff --git a/lib/common/services/hooks-service.ts b/lib/common/services/hooks-service.ts index e13ae1bfe6..c7a843e02d 100644 --- a/lib/common/services/hooks-service.ts +++ b/lib/common/services/hooks-service.ts @@ -4,7 +4,7 @@ import * as _ from "lodash"; import { annotate, getValueFromNestedObject } from "../helpers"; import { reportDeprecation } from "../deprecation"; import { createHookInvocation, isHookDefinition } from "../define-hook"; -import type { HookMiddleware, IHookDefinition } from "../define-hook"; +import type { HookMiddleware, HookDefinition } from "../define-hook"; import { runInInjectionContext } from "../di/inject"; import { AnalyticsEventLabelDelimiter } from "../../constants"; import { IOptions, IPerformanceService } from "../../declarations"; @@ -17,6 +17,7 @@ import { IErrors, IProjectHelper, IStringDictionary, + IHookExecutionOptions, } from "../declarations"; import { INsConfigHooks, @@ -99,10 +100,16 @@ export class HooksService implements IHooksService { public executeBeforeHooks( commandName: string, hookArguments?: IDictionary, - ): Promise { + options?: IHookExecutionOptions, + ): Promise { const beforeHookName = `before-${HooksService.formatHookName(commandName)}`; const traceMessage = `BeforeHookName for command ${commandName} is ${beforeHookName}`; - return this.executeHooks(beforeHookName, traceMessage, hookArguments); + return this.executeHooks( + beforeHookName, + traceMessage, + hookArguments, + !!(options && options.consumesMiddlewares), + ); } public executeAfterHooks( @@ -111,13 +118,14 @@ export class HooksService implements IHooksService { ): Promise { const afterHookName = `after-${HooksService.formatHookName(commandName)}`; const traceMessage = `AfterHookName for command ${commandName} is ${afterHookName}`; - return this.executeHooks(afterHookName, traceMessage, hookArguments); + return this.executeHooks(afterHookName, traceMessage, hookArguments, false); } private async executeHooks( hookName: string, traceMessage: string, - hookArguments?: IDictionary, + hookArguments: IDictionary, + consumesMiddlewares: boolean, ): Promise { if (this.$config.DISABLE_HOOKS || !this.$options.hooks) { return; @@ -143,6 +151,7 @@ export class HooksService implements IHooksService { hooksDirectory, hookName, hookArguments, + consumesMiddlewares, ), ); } @@ -156,6 +165,7 @@ export class HooksService implements IHooksService { hookName, hook, hookArguments, + consumesMiddlewares, ), ); } @@ -176,7 +186,8 @@ export class HooksService implements IHooksService { directoryPath: string, hookName: string, hook: IHook, - hookArguments?: IDictionary, + hookArguments: IDictionary, + consumesMiddlewares: boolean, ): Promise { hookArguments = hookArguments || {}; @@ -229,12 +240,21 @@ export class HooksService implements IHooksService { const definitionCandidate = (hookEntryPoint && hookEntryPoint.default) ?? hookEntryPoint; + // Reserved: a future release may accept several definitions from one + // file, so an array must not silently do nothing until then. + if (Array.isArray(definitionCandidate)) { + throw new Error( + `${hook.fullPath} exports an array, which is not a supported hook entry point. Export a single hook definition or function per file.`, + ); + } + if (isHookDefinition(definitionCandidate)) { result = await this.executeHookDefinition( definitionCandidate, hookName, hook, hookArguments, + consumesMiddlewares, ); } else if (typeof hookEntryPoint !== "function") { // A definition is a plain object, so this guard has to stay below the @@ -353,23 +373,37 @@ export class HooksService implements IHooksService { } private async executeHookDefinition( - definition: IHookDefinition, + definition: HookDefinition, hookName: string, hook: IHook, hookArguments: IDictionary, + consumesMiddlewares: boolean, ): Promise { + // The name decides when a hook fires, so a disagreeing one is a mistake + // with no safe reading — running it anyway would fire it at a point its + // author never wrote it for. if (definition.name !== hookName) { - this.$logger.trace( - `Hook ${hook.fullPath} defines "${definition.name}" but its file name places it at the "${hookName}" hook point; running it there.`, + this.$logger.warn( + `${hook.fullPath} will NOT be executed: it defines the "${definition.name}" hook but is placed at the "${hookName}" hook point.`, ); + return; } - const { context, middlewares } = createHookInvocation(hookArguments); + const { context, middlewares } = createHookInvocation(hookArguments, { + hookName, + consumesMiddlewares, + }); try { - await runInInjectionContext(this.$injector, () => - definition.handler(context), + const returnedValue = await runInInjectionContext(this.$injector, () => + definition.run(context), ); + + if (typeof returnedValue === "function") { + this.$logger.warn( + `${hook.fullPath} returned a function. Returning a middleware is the legacy convention and is ignored for hook definitions — use ctx.wrap() instead.`, + ); + } } catch (err) { if ( err && @@ -392,7 +426,8 @@ export class HooksService implements IHooksService { private async executeHooksInDirectory( directoryPath: string, hookName: string, - hookArguments?: IDictionary, + hookArguments: IDictionary, + consumesMiddlewares: boolean, ): Promise { hookArguments = hookArguments || {}; const results: any[] = []; @@ -405,6 +440,7 @@ export class HooksService implements IHooksService { hookName, hook, hookArguments, + consumesMiddlewares, ); if (result) { diff --git a/lib/common/test/unit-tests/stubs.ts b/lib/common/test/unit-tests/stubs.ts index 3180fedc34..8bccdc038b 100644 --- a/lib/common/test/unit-tests/stubs.ts +++ b/lib/common/test/unit-tests/stubs.ts @@ -22,7 +22,7 @@ import { export class LockServiceStub implements ILockService { public async lock( lockFilePath?: string, - lockOpts?: ILockOptions + lockOpts?: ILockOptions, ): Promise<() => void> { return () => {}; } @@ -32,7 +32,7 @@ export class LockServiceStub implements ILockService { public async executeActionWithLock( action: () => Promise, lockFilePath?: string, - lockOpts?: ILockOptions + lockOpts?: ILockOptions, ): Promise { const result = await action(); return result; @@ -107,7 +107,7 @@ export class ErrorsStub implements IErrors { async beginCommand( action: () => Promise, - printHelpCommand: () => Promise + printHelpCommand: () => Promise, ): Promise { return action(); } @@ -120,8 +120,8 @@ export class ErrorsStub implements IErrors { } export class HooksServiceStub implements IHooksService { - async executeBeforeHooks(commandName: string): Promise { - return; + async executeBeforeHooks(commandName: string): Promise { + return []; } async executeAfterHooks(commandName: string): Promise { return; @@ -153,25 +153,25 @@ export class AndroidProcessServiceStub async mapAbstractToTcpPort( deviceIdentifier: string, appIdentifier: string, - framework: string + framework: string, ): Promise { return this.MapAbstractToTcpPortResult; } async getDebuggableApps( - deviceIdentifier: string + deviceIdentifier: string, ): Promise { return this.GetDebuggableAppsResult; } async getMappedAbstractToTcpPorts( deviceIdentifier: string, appIdentifiers: string[], - framework: string + framework: string, ): Promise> { return this.GetMappedAbstractToTcpPortsResult; } async getAppProcessId( deviceIdentifier: string, - appIdentifier: string + appIdentifier: string, ): Promise { while (this.GetAppProcessIdFailAttempts) { this.GetAppProcessIdFailAttempts--; @@ -181,7 +181,7 @@ export class AndroidProcessServiceStub return this.GetAppProcessIdResult; } async forwardFreeTcpToAbstractPort( - portForwardInputData: Mobile.IPortForwardData + portForwardInputData: Mobile.IPortForwardData, ): Promise { return this.ForwardFreeTcpToAbstractPortResult; } diff --git a/lib/contracts/index.ts b/lib/contracts/index.ts index c65b022042..6c63e33817 100644 --- a/lib/contracts/index.ts +++ b/lib/contracts/index.ts @@ -28,8 +28,9 @@ export { ProjectNameService } from "./project-name-service"; export { defineHook, isHookDefinition } from "../common/define-hook"; export type { - IHookContext, - IHookDefinition, + HookContext, + HookDefinition, + HookDefinitionInput, HookHandler, HookMiddleware, } from "../common/define-hook"; diff --git a/test/define-hook.ts b/test/define-hook.ts index d3030f6b96..e6bc7c3f70 100644 --- a/test/define-hook.ts +++ b/test/define-hook.ts @@ -8,6 +8,7 @@ import { hook } from "../lib/common/helpers"; import { IInjector } from "../lib/common/definitions/yok"; import { IHooksService } from "../lib/common/declarations"; import { LoggerStub, ErrorsStub } from "./stubs"; +import { defineHook, isHookDefinition } from "../lib/common/define-hook"; // Hook fixtures load the API the way a real hook does — through the published // `nativescript/contracts` entry point — so the marker symbol, the context @@ -308,7 +309,7 @@ describe("defineHook", () => { assert.strictEqual(capture.payload, payload); }); - it("runs a definition whose name differs from the hook point and traces the mismatch", async () => { + it("skips a definition whose name differs from the hook point, with a warning", async () => { writeHook( projectDir, "before-case11", @@ -320,8 +321,173 @@ describe("defineHook", () => { await hooksService().executeBeforeHooks("case11"); - assert.isTrue(capture.ran); - assert.include(logger().traceOutput, `defines "before-something-else"`); - assert.include(logger().traceOutput, `"before-case11" hook point`); + assert.isUndefined(capture.ran); + assert.include(logger().warnOutput, `defines the "before-something-else"`); + assert.include(logger().warnOutput, `"before-case11" hook point`); + }); + + it("accepts the object bag form", async () => { + writeHook( + projectDir, + "before-case12", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook({ + name: "before-case12", + run: (ctx) => { + global.__hookCapture.payload = ctx.payload; + }, + });`, + ); + + const payload = { fromBag: true }; + await hooksService().executeBeforeHooks("case12", { hookArgs: payload }); + + assert.strictEqual(capture.payload, payload); + }); + + it("rejects a wrap() at a hook point that consumes no middlewares", async () => { + writeHook( + projectDir, + "before-case13", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case13", (ctx) => { + ctx.wrap((args, next) => next(...args)); + });`, + ); + + await assert.isRejected( + hooksService().executeBeforeHooks("case13"), + /ctx\.wrap\(\) is not available at the "before-case13" hook point/, + ); + }); + + it("rejects a wrap() from an after-hook", async () => { + writeHook( + projectDir, + "after-case14", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("after-case14", (ctx) => { + ctx.wrap((args, next) => next(...args)); + });`, + ); + + await assert.isRejected( + hooksService().executeAfterHooks("case14"), + /ctx\.wrap\(\) is not available at the "after-case14" hook point/, + ); + }); + + it("defaults the abort() message instead of failing with Error(undefined)", async () => { + writeHook( + projectDir, + "before-case15", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case15", (ctx) => { + ctx.abort(); + });`, + ); + + await assert.isRejected( + hooksService().executeBeforeHooks("case15"), + /The "before-case15" hook aborted without a message\./, + ); + }); + + it("warns when a definition returns a function instead of calling ctx.wrap()", async () => { + writeHook( + projectDir, + "before-case16", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case16", () => { + return () => "legacy middleware"; + });`, + ); + + await hooksService().executeBeforeHooks("case16"); + + assert.include(logger().warnOutput, "returned a function"); + }); + + it("rejects an array export, naming the file", async () => { + const fullPath = writeHook( + projectDir, + "before-case17", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = [defineHook("before-case17", () => {})];`, + ); + + await assert.isRejected( + hooksService().executeBeforeHooks("case17"), + new RegExp( + `${fullPath.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")} exports an array`, + ), + ); + }); +}); + +describe("defineHook validation", () => { + // The negative cases are exactly the ones the types reject, so they need an + // untyped view of the same function. + const defineHookUnsafe: any = defineHook; + + it("carries the payload generic through to ctx.payload", () => { + const definition = defineHook<{ args: string[] }>( + "before-build-task-args", + (ctx) => { + // Compile-time: `payload` is `{ args: string[] } | undefined`, so it + // needs narrowing before use. + assert.isUndefined(ctx.payload?.args); + }, + ); + + assert.isTrue(isHookDefinition(definition)); + }); + + it("rejects a bag with an unknown field, naming it and the accepted forms", () => { + assert.throws( + () => defineHookUnsafe({ name: "before-prepare", handler: () => {} }), + /unknown field "handler".*Supported fields: "name", "run".*Accepted forms/s, + ); + }); + + it("rejects a bag with no run", () => { + assert.throws( + () => defineHookUnsafe({ name: "before-prepare" }), + /"before-prepare".*requires "run" to be a function/, + ); + }); + + it("rejects a bag with no name", () => { + assert.throws( + () => defineHookUnsafe({ run: () => {} }), + /requires a non-empty "name"/, + ); + }); + + it("rejects the positional form without a handler function", () => { + assert.throws( + () => defineHookUnsafe("before-prepare"), + /"before-prepare".*requires a handler function as its second argument/, + ); + }); + + it("rejects a non-object, non-string argument", () => { + assert.throws( + () => defineHookUnsafe(undefined), + /called with an unsupported argument/, + ); + }); + + it("keeps the marker through a spread, so derived definitions stay recognizable", () => { + const definition = defineHook("before-prepare", () => {}); + const derived = { ...definition, name: "before-build" }; + + assert.isTrue(isHookDefinition(definition)); + assert.isTrue(isHookDefinition(derived)); + assert.equal(derived.name, "before-build"); + }); + + it("does not recognize a hand-rolled object", () => { + assert.isFalse(isHookDefinition({ name: "before-prepare", run: () => {} })); }); }); diff --git a/test/stubs.ts b/test/stubs.ts index 29545654d8..c815f5ba54 100644 --- a/test/stubs.ts +++ b/test/stubs.ts @@ -386,9 +386,7 @@ export class ErrorsStub implements IErrors { ): void {} } -export class PackageInstallationManagerStub - implements IPackageInstallationManager -{ +export class PackageInstallationManagerStub implements IPackageInstallationManager { clearInspectorCache(): void { return undefined; } @@ -735,9 +733,7 @@ export class ProjectDataStub implements IProjectData { } } -export class AndroidPluginBuildServiceStub - implements IAndroidPluginBuildService -{ +export class AndroidPluginBuildServiceStub implements IAndroidPluginBuildService { buildAar(options: IPluginBuildOptions): Promise { return Promise.resolve(true); } @@ -1002,8 +998,8 @@ export class ProjectTemplatesService implements IProjectTemplatesService { } export class HooksServiceStub implements IHooksService { - async executeBeforeHooks(commandName: string): Promise { - return Promise.resolve(); + async executeBeforeHooks(commandName: string): Promise { + return Promise.resolve([]); } async executeAfterHooks(commandName: string): Promise { @@ -1313,9 +1309,7 @@ export class CommandsService implements ICommandsService { } } -export class AndroidResourcesMigrationServiceStub - implements IAndroidResourcesMigrationService -{ +export class AndroidResourcesMigrationServiceStub implements IAndroidResourcesMigrationService { canMigrate(platformString: string): boolean { return true; } @@ -1329,9 +1323,7 @@ export class AndroidResourcesMigrationServiceStub } } -export class AndroidBundleValidatorHelper - implements IAndroidBundleValidatorHelper -{ +export class AndroidBundleValidatorHelper implements IAndroidBundleValidatorHelper { validateDeviceApiLevel(device: Mobile.IDevice, buildData: IBuildData): void { return; } From 823e9cba8287d9330853313d677702880d33b8f4 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 15:36:38 -0300 Subject: [PATCH 4/5] docs(hooks): lead with the reworked defineHook API and correct the contract Documents the bag form, define-time validation, the strict name match and the one-definition-per-file rule; splits ctx into payload/wrap/abort sections; states that dispatch-fired hook points carry no payload and lists the hook points where wrap() is honored. Corrects two long-standing errors: a hook named plainly `watch` never fires (the points are `before-watch`/`after-watch`), and downgrading a rejection to a warning needs `errorAsWarning === true` together with a Boolean `stopExecution`, not `stopExecution: false` alone. --- extending-cli.md | 73 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 55 insertions(+), 18 deletions(-) diff --git a/extending-cli.md b/extending-cli.md index fec73320ca..31eb525369 100644 --- a/extending-cli.md +++ b/extending-cli.md @@ -11,7 +11,7 @@ For the NativeScript CLI to execute your hooks, you must place them in the `hook You can attach the hook before or after `prepare` operations or to `--watch` operations. -Note that `watch` hooks can be executed only at the time of running `--watch` operations. The `watch` hooks are the last thing executed before launching the file system watcher which tracks for changes to your code. +Note that `watch` hooks can be executed only at the time of running `--watch` operations. The `before-watch` hooks are the last thing executed before launching the file system watcher which tracks for changes to your code. Your hooks must conform to the following naming and placement conventions: @@ -36,27 +36,29 @@ Your hooks must conform to the following naming and placement conventions: ├── hook1 (this is an executable file) └── hook2 (this is an executable file) ``` -* If you want to attach a hook for `--watch` operations, you must place the hook in the root of the `hooks` subdirectory. The file must be named `watch`. For example: +* If you want to attach a hook for `--watch` operations, you must place the hook in the root of the `hooks` subdirectory. The file must be named `before-watch` or `after-watch`. For example: ``` my-app/ ├── index.js ├── package.json └── hooks/ - └── watch.js (this is a Node.js script) + └── before-watch.js (this is a Node.js script) ``` -* If you want to attach multiple hooks for `--watch` operations, you must place them inside a `watch` subdirectory of the `hooks` subdirectory. You can specify any meaningful name for the the hooks inside the subdirectory. For example: +* If you want to attach multiple hooks for `--watch` operations, you must place them inside a `before-watch` or `after-watch` subdirectory of the `hooks` subdirectory. You can specify any meaningful name for the the hooks inside the subdirectory. For example: ``` my-app/ ├── index.js ├── package.json └── hooks/ - └── watch (a directory) + └── before-watch (a directory) ├── hook1 (this is an executable file) └── hook2 (this is an executable file) ``` + A file named plainly `watch` is never executed: like every other hook point, the watch hooks are addressed by the `before-`/`after-` names above. + > **NOTE:** When multiple hooks are attached to a single event (i.e. multiple hooks are stored in dedicated subdirectories), at the specified time, the CLI executes each hook one by one. However, the order of hook execution is not strict and might change over command executions. Execute Hooks as Child Process @@ -81,17 +83,30 @@ The CLI assumes that this is a CommonJS module and calls the hook it exports — ## Writing a hook -Export a hook definition built with `defineHook`. It takes the hook point in the usual naming convention (`before-prepare`, `after-watch`) and a handler that receives a context object. +Export a hook definition built with `defineHook`. It takes the hook point in the usual naming convention (`before-prepare`, `after-watch`) and a `run` handler that receives a context object. ```JavaScript const { defineHook, inject, DoctorService } = require("nativescript/contracts"); -module.exports = defineHook("before-prepare", async (ctx) => { - const doctorService = inject(DoctorService); - await doctorService.canExecuteLocalBuild(); +module.exports = defineHook({ + name: "before-prepare", + run: async (ctx) => { + const doctorService = inject(DoctorService); + await doctorService.canExecuteLocalBuild(); + }, }); ``` +`defineHook(name, run)` is shorthand for the same definition: + +```JavaScript +module.exports = defineHook("before-prepare", async (ctx) => { /* ... */ }); +``` + +`defineHook` validates its input immediately: a missing or non-string `name`, a missing or non-function `run`, and unknown fields all throw at definition time, naming the definition and both accepted forms. + +The `name` decides when the hook fires and must match the hook point the file is placed at. A definition whose `name` disagrees with its location is **skipped with a warning** rather than run at the wrong point. Export exactly one definition (or one plain function) per file — an array export is rejected. + Services come from `inject()` — the same API used everywhere else (see [dependency-injection.md](dependency-injection.md)): * `inject()` is valid in the synchronous part of the handler — not after an `await`. Resolve what you need up front; for late lookups, grab the container first: `const injector = inject(Injector)` (`Injector` is exported from `nativescript/contracts` too), then `injector.get(...)` later. @@ -99,6 +114,8 @@ Services come from `inject()` — the same API used everywhere else (see [depend * Only a first tranche of services has typed tokens so far ([dependency-injection.md](dependency-injection.md#available-contracts) lists them); a service without a token is reachable by its registry name — `inject("logger")` — as a migration bridge. * If you build your hook in TypeScript, add `nativescript` as a `devDependency` and import the same names: `import { defineHook, inject, DoctorService } from "nativescript/contracts"`. An `.mjs` hook can `export default defineHook(...)`. +### `ctx.payload` + `ctx.payload` holds the parameters of the CLI operation being hooked; its shape depends on the hook point. It is the CLI's own object, so mutating it influences the operation: ```JavaScript @@ -107,7 +124,19 @@ module.exports = defineHook("before-build-task-args", (ctx) => { }); ``` -`ctx.wrap(middleware)` puts a middleware around the hooked method. The middleware receives the method's arguments and a `next` callback; call `next` to continue, or return without calling it to short-circuit the method entirely. Register it from a `before-` hook. +Not every invocation carries one. The `before-`/`after-` hooks fired around command dispatch (`before-build`, `after-run`, …) pass no arguments at all, so `ctx.payload` is `undefined` there. Treat it as optional — in TypeScript it is typed `TPayload | undefined`: + +```TypeScript +import { defineHook } from "nativescript/contracts"; + +export default defineHook<{ args: string[] }>("before-build-task-args", (ctx) => { + ctx.payload?.args.push("--offline"); +}); +``` + +### `ctx.wrap(middleware)` + +`ctx.wrap(middleware)` puts a middleware around the hooked method. The middleware receives the method's arguments and a `next` callback; call `next` to continue, or return without calling it to short-circuit the method entirely. ```JavaScript module.exports = defineHook("before-prepare", (ctx) => { @@ -118,7 +147,15 @@ module.exports = defineHook("before-prepare", (ctx) => { }); ``` -`ctx.abort(message)` stops the hook and fails the command. Pass `{ asWarning: true }` to print the message as a warning and let the command continue instead. +Only a hook point that actually folds middlewares around a method can honor `wrap()`, so it is available **only in the before-phase of the wrappable hook points** listed below. Calling it anywhere else — from any `after-` hook, or from a before-hook at a non-wrappable point — throws an error naming the hook point instead of registering a middleware that would never run. + +The wrappable hook points are: + +`before-buildAndroid` · `before-buildAndroidPlugin` · `before-buildIOS` · `before-checkEnvironment` · `before-checkForChanges` · `before-install` · `before-prepare` · `before-prepareNativeApp` · `before-resolveCommand` · `before-watch` · `before-watchPatterns` + +### `ctx.abort(message)` + +`ctx.abort(message)` stops the hook and fails the command. Pass `{ asWarning: true }` to print the message as a warning and let the command continue instead. The message is required in practice — calling `abort()` without one falls back to a message naming the hook point. ```JavaScript module.exports = defineHook("before-prepare", (ctx) => { @@ -142,18 +179,18 @@ module.exports = function (hookArgs) { ## The hook contract The hook must return a Promise. If the hook succeeds, it must fullfil the promise, but the fullfilment value is ignored. -The hook can also reject the promise with an instance of Error. The returned error can have two optional members controlling the CLI. - +The hook can also reject the promise with an instance of Error. The returned error can carry two members that together downgrade the rejection to a warning. + Member | Type | Description ---|---|--- -`stopExecution` | Boolean | Set this to `false` to let the CLI continue executing this command. -`errorAsWarning` | Boolean | Set this to treat the returned error as warning. The CLI prints the error.message colored as a warning and continues executing the current command. - -If these two members are not set, the CLI prints the returned error colored as fatal error and stops executing the current command. +`errorAsWarning` | Boolean | Must be exactly `true`. The CLI prints the error.message colored as a warning and continues executing the current command. +`stopExecution` | Boolean | Must be present and of type Boolean. It only enables the check — setting it alone, with either value, changes nothing. + +**Both** members are required: the CLI continues only when `errorAsWarning === true` *and* `stopExecution` is a Boolean. Otherwise it prints the returned error colored as a fatal error and stops executing the current command. A plain-function hook can also return a function, which the CLI folds into a middleware chain around the hooked method. -With `defineHook` neither convention is needed: `ctx.abort` replaces throwing an error carrying `stopExecution`/`errorAsWarning`, and `ctx.wrap` replaces returning a function. +With `defineHook` neither convention is needed, and neither applies: `ctx.abort` replaces throwing an error carrying `stopExecution`/`errorAsWarning`, and `ctx.wrap` replaces returning a function. A definition whose `run` returns a function is warned about — the returned function is not used as a middleware. ## Legacy: parameter-name injection From 26e6639546348052b4a723e411494f1a67d5e09a Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 5 Aug 2026 16:15:48 -0300 Subject: [PATCH 5/5] refactor(hooks): split ctx.abort into ctx.fail and ctx.skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `abort(message, { asWarning: true })` did not abort anything — it warned and the command carried on, so one verb meant opposite things depending on a flag. The two outcomes now have a verb each: `ctx.fail(message)` fails the command, `ctx.skip(message)` warns and lets it continue. Both still stop the handler by throwing, so both are typed `never` and the "handler ends here" behavior is unchanged; only the command's fate differs. A missing message falls back to one naming the hook point and the method. `abort` and the `asWarning` option are removed outright rather than shimmed, as the API is unreleased and never exposed either name. --- extending-cli.md | 20 +++++++++++++---- lib/common/define-hook.ts | 45 +++++++++++++++++++++++++++------------ test/define-hook.ts | 40 ++++++++++++++++++++++++++-------- 3 files changed, 78 insertions(+), 27 deletions(-) diff --git a/extending-cli.md b/extending-cli.md index 31eb525369..8808a4fde1 100644 --- a/extending-cli.md +++ b/extending-cli.md @@ -153,16 +153,28 @@ The wrappable hook points are: `before-buildAndroid` · `before-buildAndroidPlugin` · `before-buildIOS` · `before-checkEnvironment` · `before-checkForChanges` · `before-install` · `before-prepare` · `before-prepareNativeApp` · `before-resolveCommand` · `before-watch` · `before-watchPatterns` -### `ctx.abort(message)` +### `ctx.fail(message)` and `ctx.skip(message)` -`ctx.abort(message)` stops the hook and fails the command. Pass `{ asWarning: true }` to print the message as a warning and let the command continue instead. The message is required in practice — calling `abort()` without one falls back to a message naming the hook point. +Both end the handler immediately — nothing after the call runs — and differ in what happens to the command. + +`ctx.fail(message)` fails the command, printing `message` as the error: + +```JavaScript +module.exports = defineHook("before-prepare", (ctx) => { + ctx.fail("The generated bundle is missing; run the bundler first."); +}); +``` + +`ctx.skip(message)` prints `message` as a warning and lets the command continue: ```JavaScript module.exports = defineHook("before-prepare", (ctx) => { - ctx.abort("Nothing to prepare.", { asWarning: true }); + ctx.skip("Nothing to prepare."); }); ``` +The message is required in practice — calling either without one falls back to a message naming the hook point and the method. + ### Plain function hooks Exporting a plain function is still supported. It runs in an injection context too, so `inject()` works the same way; declare a `hookArgs` parameter if you need the payload. @@ -190,7 +202,7 @@ Member | Type | Description A plain-function hook can also return a function, which the CLI folds into a middleware chain around the hooked method. -With `defineHook` neither convention is needed, and neither applies: `ctx.abort` replaces throwing an error carrying `stopExecution`/`errorAsWarning`, and `ctx.wrap` replaces returning a function. A definition whose `run` returns a function is warned about — the returned function is not used as a middleware. +With `defineHook` neither convention is needed, and neither applies: `ctx.fail`/`ctx.skip` replace throwing an error carrying `stopExecution`/`errorAsWarning`, and `ctx.wrap` replaces returning a function. A definition whose `run` returns a function is warned about — the returned function is not used as a middleware. ## Legacy: parameter-name injection diff --git a/lib/common/define-hook.ts b/lib/common/define-hook.ts index 1da83c94bb..c4f0b8e03f 100644 --- a/lib/common/define-hook.ts +++ b/lib/common/define-hook.ts @@ -42,10 +42,20 @@ export interface HookContext { wrap(middleware: HookMiddleware): void; /** - * Stops the hook. With `asWarning`, the CLI logs the message and continues - * the command; otherwise the command fails. + * Ends the handler and fails the command with `message`. + * + * Typed `never` because it stops the handler by throwing, so nothing after + * the call runs. */ - abort(message: string, opts?: { asWarning?: boolean }): never; + fail(message: string): never; + + /** + * Ends the handler and logs `message` as a warning; the command continues. + * + * Typed `never` because it stops the handler by throwing, so nothing after + * the call runs — only the command outlives it. + */ + skip(message: string): never; } export type HookHandler = ( @@ -209,17 +219,14 @@ export function createHookInvocation( middlewares.push(middleware); }, - abort(message: string, opts?: { asWarning?: boolean }): never { - const text = - typeof message === "string" && message.trim().length - ? message - : `The "${hookName}" hook aborted without a message.`; - const error: any = new Error(text); - if (opts && opts.asWarning) { - // The pair the hooks service checks for to downgrade a rejection. - error.stopExecution = false; - error.errorAsWarning = true; - } + fail(message: string): never { + throw new Error(hookMessage(message, hookName, "fail")); + }, + skip(message: string): never { + const error: any = new Error(hookMessage(message, hookName, "skip")); + // The pair the hooks service checks for to downgrade a rejection. + error.stopExecution = false; + error.errorAsWarning = true; throw error; }, }; @@ -227,6 +234,16 @@ export function createHookInvocation( return { context, middlewares }; } +function hookMessage( + message: string, + hookName: string, + method: string, +): string { + return typeof message === "string" && message.trim().length + ? message + : `The "${hookName}" hook called ctx.${method}() without a message.`; +} + function derivePayload(hookArguments: any): any { if (!hookArguments || typeof hookArguments !== "object") { return undefined; diff --git a/test/define-hook.ts b/test/define-hook.ts index e6bc7c3f70..22bac0974d 100644 --- a/test/define-hook.ts +++ b/test/define-hook.ts @@ -224,35 +224,39 @@ describe("defineHook", () => { assert.isUndefined(capture.originalRan); }); - it("logs a warning and continues when the handler aborts with asWarning", async () => { + it("warns and continues the command when the handler skips, stopping the handler", async () => { writeHook( projectDir, "before-case7", `const { defineHook } = require(${JSON.stringify(apiPath)}); module.exports = defineHook("before-case7", async (ctx) => { - ctx.abort("soft-abort", { asWarning: true }); + ctx.skip("soft-skip"); + global.__hookCapture.afterSkip = true; });`, ); await hooksService().executeBeforeHooks("case7"); - assert.include(logger().warnOutput, "soft-abort"); + assert.include(logger().warnOutput, "soft-skip"); + assert.isUndefined(capture.afterSkip); }); - it("fails the command when the handler aborts without asWarning", async () => { + it("fails the command when the handler fails, stopping the handler", async () => { writeHook( projectDir, "before-case8", `const { defineHook } = require(${JSON.stringify(apiPath)}); module.exports = defineHook("before-case8", async (ctx) => { - ctx.abort("hard-abort"); + ctx.fail("hard-fail"); + global.__hookCapture.afterFail = true; });`, ); await assert.isRejected( hooksService().executeBeforeHooks("case8"), - /hard-abort/, + /hard-fail/, ); + assert.isUndefined(capture.afterFail); }); it("keeps a legacy param-name hook on the old path, and never reports a definition hook", async () => { @@ -377,19 +381,37 @@ describe("defineHook", () => { ); }); - it("defaults the abort() message instead of failing with Error(undefined)", async () => { + it("defaults the fail() message instead of failing with Error(undefined)", async () => { writeHook( projectDir, "before-case15", `const { defineHook } = require(${JSON.stringify(apiPath)}); module.exports = defineHook("before-case15", (ctx) => { - ctx.abort(); + ctx.fail(); });`, ); await assert.isRejected( hooksService().executeBeforeHooks("case15"), - /The "before-case15" hook aborted without a message\./, + /The "before-case15" hook called ctx\.fail\(\) without a message\./, + ); + }); + + it("defaults the skip() message", async () => { + writeHook( + projectDir, + "before-case18", + `const { defineHook } = require(${JSON.stringify(apiPath)}); + module.exports = defineHook("before-case18", (ctx) => { + ctx.skip(); + });`, + ); + + await hooksService().executeBeforeHooks("case18"); + + assert.include( + logger().warnOutput, + 'The "before-case18" hook called ctx.skip() without a message.', ); });