diff --git a/extending-cli.md b/extending-cli.md index 5ace718f18..8808a4fde1 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 @@ -77,11 +79,105 @@ 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 `run` handler that receives a context object. + +```JavaScript +const { defineHook, inject, DoctorService } = require("nativescript/contracts"); + +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. +* 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` + +`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"); +}); +``` + +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) => { + ctx.wrap(async (args, next) => { + const result = await next(...args); + return result; + }); +}); +``` + +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.fail(message)` and `ctx.skip(message)` + +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.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. ```JavaScript const { inject, DoctorService } = require("nativescript/contracts"); @@ -92,23 +188,21 @@ 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. -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, 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/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 new file mode 100644 index 0000000000..c4f0b8e03f --- /dev/null +++ b/lib/common/define-hook.ts @@ -0,0 +1,257 @@ +/** + * 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. + * + * 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", +); + +/** + * 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 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. Hook points fired by command + * dispatch carry no payload at all, hence `undefined`. + */ + payload: TPayload | undefined; + + /** + * 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; + + /** + * Ends the handler and fails the command with `message`. + * + * Typed `never` because it stops the handler by throwing, so nothing after + * the call runs. + */ + 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 = ( + 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 HookDefinition { + /** Hook point, in the hyphen convention: `before-prepare`, `after-watch`. */ + readonly name: string; + readonly run: HookHandler; +} + +export interface HookInvocation { + context: HookContext; + /** Populated by `ctx.wrap()` while the handler runs. */ + middlewares: HookMiddleware[]; +} + +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, + 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; +} + +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.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, + options: HookInvocationOptions, +): HookInvocation { + const { hookName, consumesMiddlewares } = options; + const middlewares: HookMiddleware[] = []; + 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); + }, + 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; + }, + }; + + 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; + } + + if ("hookArgs" in hookArguments) { + return hookArguments["hookArgs"]; + } + + return Object.keys(hookArguments).length ? hookArguments : undefined; +} 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 b1638ede0c..c7a843e02d 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, HookDefinition } from "../define-hook"; +import { runInInjectionContext } from "../di/inject"; import { AnalyticsEventLabelDelimiter } from "../../constants"; import { IOptions, IPerformanceService } from "../../declarations"; import { @@ -14,6 +17,7 @@ import { IErrors, IProjectHelper, IStringDictionary, + IHookExecutionOptions, } from "../declarations"; import { INsConfigHooks, @@ -96,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( @@ -108,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; @@ -140,6 +151,7 @@ export class HooksService implements IHooksService { hooksDirectory, hookName, hookArguments, + consumesMiddlewares, ), ); } @@ -153,6 +165,7 @@ export class HooksService implements IHooksService { hookName, hook, hookArguments, + consumesMiddlewares, ), ); } @@ -173,7 +186,8 @@ export class HooksService implements IHooksService { directoryPath: string, hookName: string, hook: IHook, - hookArguments?: IDictionary, + hookArguments: IDictionary, + consumesMiddlewares: boolean, ): Promise { hookArguments = hookArguments || {}; @@ -221,80 +235,105 @@ export class HooksService implements IHooksService { : hookModule; } - if (typeof hookEntryPoint !== "function") { - this.$logger.warn( - `${hook.fullPath} will NOT be executed because it does not export a 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; + + // 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.`, ); - return; } - this.$logger.trace(`Validating ${hookName} arguments.`); - - const invalidArguments = this.validateHookArguments( - hookEntryPoint, - hook.fullPath, - ); - - if (invalidArguments.length) { + 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 + // definition check. this.$logger.warn( - `${ - hook.fullPath - } will NOT be executed because it has invalid arguments - ${color.grey( - invalidArguments.join(", "), - )}.`, + `${hook.fullPath} will NOT be executed because it does not export a function.`, ); return; - } + } else { + this.$logger.trace(`Validating ${hookName} arguments.`); - // 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 invalidArguments = this.validateHookArguments( + hookEntryPoint, + 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, - }); - } + if (invalidArguments.length) { + this.$logger.warn( + `${ + hook.fullPath + } will NOT be executed because it has invalid arguments - ${color.grey( + invalidArguments.join(", "), + )}.`, + ); + return; + } - 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}.`); - } + // 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; } - this.$logger.trace("Hook completed"); + // 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, + }); + } + + 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,10 +372,62 @@ export class HooksService implements IHooksService { return result; } + private async executeHookDefinition( + 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.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, { + hookName, + consumesMiddlewares, + }); + + try { + 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 && + _.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, - hookArguments?: IDictionary, + hookArguments: IDictionary, + consumesMiddlewares: boolean, ): Promise { hookArguments = hookArguments || {}; const results: any[] = []; @@ -349,6 +440,7 @@ export class HooksService implements IHooksService { hookName, hook, hookArguments, + consumesMiddlewares, ); if (result) { @@ -356,7 +448,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/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 c13a9b9996..6c63e33817 100644 --- a/lib/contracts/index.ts +++ b/lib/contracts/index.ts @@ -25,3 +25,12 @@ export type { export { DoctorService } from "./doctor-service"; export { ProjectNameService } from "./project-name-service"; + +export { defineHook, isHookDefinition } from "../common/define-hook"; +export type { + HookContext, + HookDefinition, + HookDefinitionInput, + HookHandler, + HookMiddleware, +} from "../common/define-hook"; diff --git a/test/define-hook.ts b/test/define-hook.ts new file mode 100644 index 0000000000..22bac0974d --- /dev/null +++ b/test/define-hook.ts @@ -0,0 +1,515 @@ +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"; +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 +// 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); + 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("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.skip("soft-skip"); + global.__hookCapture.afterSkip = true; + });`, + ); + + await hooksService().executeBeforeHooks("case7"); + + assert.include(logger().warnOutput, "soft-skip"); + assert.isUndefined(capture.afterSkip); + }); + + 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.fail("hard-fail"); + global.__hookCapture.afterFail = true; + });`, + ); + + await assert.isRejected( + hooksService().executeBeforeHooks("case8"), + /hard-fail/, + ); + assert.isUndefined(capture.afterFail); + }); + + 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("skips a definition whose name differs from the hook point, with a warning", 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.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 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.fail(); + });`, + ); + + await assert.isRejected( + hooksService().executeBeforeHooks("case15"), + /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.', + ); + }); + + 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; }