feat(hooks): defineHook — typed hook authoring with ctx payload/wrap/fail/skip - #6100
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds a typed ChangesTyped hook execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DecoratedCommand
participant HooksService
participant HookDefinition
participant HookInvocation
DecoratedCommand->>HooksService: executeBeforeHooks with middleware consumption
HooksService->>HookDefinition: validate and execute hook
HookDefinition->>HookInvocation: create context and register middleware
HookInvocation-->>HooksService: return middleware and control result
HooksService-->>DecoratedCommand: return collected middleware
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
edusperoni
force-pushed
the
feat/define-hook
branch
5 times, most recently
from
July 30, 2026 02:51
514c272 to
077c41b
Compare
edusperoni
force-pushed
the
feat/define-hook
branch
2 times, most recently
from
August 5, 2026 19:22
e932eec to
cd153f4
Compare
edusperoni
changed the title
edusperoni
marked this pull request as ready for review
August 5, 2026 20:22
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/common/services/hooks-service.ts (1)
129-132: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
executeBeforeHookspromises aHookMiddleware[]but can resolveundefined. The declared contract is non-nullable, whileexecuteHooksreturns with no value on the disabled-hooks path.decorateMethodmasks this today with itsif (newMethods && newMethods.length)guard, so any new caller that trusts the type and calls.lengthor.filterthrows aTypeErrorwhenDISABLE_HOOKSis set or--no-hooksis used.
lib/common/services/hooks-service.ts#L129-L132: return[]instead of a barereturn, and change theexecuteHooksreturn type toPromise<any[]>.lib/common/declarations.d.ts#L838-L843: keepPromise<HookMiddleware[]>once the service always resolves an array; no change is needed here after the service fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/common/services/hooks-service.ts` around lines 129 - 132, Update executeHooks in lib/common/services/hooks-service.ts to return an empty array, rather than undefined, when hooks are disabled or unavailable, and change its return type to Promise<any[]> so executeBeforeHooks always resolves an array. Keep lib/common/declarations.d.ts lines 838-843 unchanged with Promise<HookMiddleware[]>; it requires no direct change because the service fix satisfies that contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/define-hook.ts`:
- Around line 286-294: Make the deprecation test deterministic by capturing the
existing process.env.NS_DEPRECATIONS value, setting the stage to the default
trace-compatible value for the test, and restoring or deleting it in a finally
block. Update the test containing deprecationReports so restoration occurs even
if reportDeprecation or an assertion throws.
---
Outside diff comments:
In `@lib/common/services/hooks-service.ts`:
- Around line 129-132: Update executeHooks in
lib/common/services/hooks-service.ts to return an empty array, rather than
undefined, when hooks are disabled or unavailable, and change its return type to
Promise<any[]> so executeBeforeHooks always resolves an array. Keep
lib/common/declarations.d.ts lines 838-843 unchanged with
Promise<HookMiddleware[]>; it requires no direct change because the service fix
satisfies that contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c965e85-3567-444b-972f-af9f3700b028
📒 Files selected for processing (9)
extending-cli.mdlib/common/declarations.d.tslib/common/define-hook.tslib/common/helpers.tslib/common/services/hooks-service.tslib/common/test/unit-tests/stubs.tslib/contracts/index.tstest/define-hook.tstest/stubs.ts
Comment on lines
+286
to
+294
| 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), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This assertion depends on the ambient NS_DEPRECATIONS value.
reportDeprecation selects the output channel from getDeprecationStage(). This test reads traceOutput, which only holds the message in the default stage. If a developer or a CI job exports NS_DEPRECATIONS=warn, the message goes to warnOutput and the assertion at line 289 fails. With NS_DEPRECATIONS=error the call throws instead.
Pin the variable for this test so the channel is deterministic.
💚 Proposed fix: pin the deprecation stage for this test
it("keeps a legacy param-name hook on the old path, and never reports a definition hook", async () => {
+ const previousStage = process.env.NS_DEPRECATIONS;
+ delete process.env.NS_DEPRECATIONS;
+ try {
const legacyPath = writeHookInDirectory(Restore the value in a finally block at the end of the test:
} finally {
if (previousStage === undefined) {
delete process.env.NS_DEPRECATIONS;
} else {
process.env.NS_DEPRECATIONS = previousStage;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/define-hook.ts` around lines 286 - 294, Make the deprecation test
deterministic by capturing the existing process.env.NS_DEPRECATIONS value,
setting the stage to the default trace-compatible value for the test, and
restoring or deleting it in a finally block. Update the test containing
deprecationReports so restoration occurs even if reportDeprecation or an
assertion throws.
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.
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.
…-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<TPayload>` / `HookContext<TPayload>` type the payload, as
`TPayload | undefined` because dispatch-fired hooks carry none, and
executeBeforeHooks is typed with the middleware array it already returns.
…ntract 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.
`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.
NathanWalker
force-pushed
the
feat/define-hook
branch
from
August 6, 2026 02:43
cd153f4 to
26e6639
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
NathanWalker
approved these changes
Aug 6, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extending-cli.md`:
- Line 82: Update the hook module description around the CommonJS statement to
make the CommonJS export rule conditional rather than universal. Add a separate
description for .mjs hooks that use an export default, while preserving the
existing hook definition and plain-function behavior.
- Around line 194-205: Update the defineHook handler contract near the hook
rejection behavior to allow synchronous run handlers, stating that the CLI
awaits the handler result rather than requiring every handler to return a
Promise. If the Promise requirement is retained, scope it explicitly to the
legacy plain-function hook API and keep defineHook compatible with the
synchronous examples.
In `@test/define-hook.ts`:
- Around line 125-129: Update the test around hooksService().executeAfterHooks
to store the full argument bag in a payload variable, pass that same variable to
executeAfterHooks, and assert capture.payload is strictly identical to payload.
Replace the nested liveSyncResultInfo identity assertion so shallow-copy
implementations no longer satisfy the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97cf36b8-cc31-463a-b97e-b992bd7382a3
📒 Files selected for processing (9)
extending-cli.mdlib/common/declarations.d.tslib/common/define-hook.tslib/common/helpers.tslib/common/services/hooks-service.tslib/common/test/unit-tests/stubs.tslib/contracts/index.tstest/define-hook.tstest/stubs.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/common/helpers.ts
- lib/contracts/index.ts
- lib/common/declarations.d.ts
- test/stubs.ts
- lib/common/test/unit-tests/stubs.ts
- lib/common/define-hook.ts
- lib/common/services/hooks-service.ts
| 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. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe CommonJS hooks conditionally.
Line 82 says that every Node.js hook is a CommonJS module. Line 115 documents .mjs hooks with export default. State the CommonJS export rule conditionally, then document .mjs default exports separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extending-cli.md` at line 82, Update the hook module description around the
CommonJS statement to make the CommonJS export rule conditional rather than
universal. Add a separate description for .mjs hooks that use an export default,
while preserving the existing hook definition and plain-function behavior.
Comment on lines
+194
to
+205
| 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. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not require a Promise from every defineHook handler.
Line 193 requires every hook to return a Promise. The new examples use synchronous run handlers. Change the contract to state that the CLI awaits the handler result, or limit the Promise requirement to the legacy API if that is the intended contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extending-cli.md` around lines 194 - 205, Update the defineHook handler
contract near the hook rejection behavior to allow synchronous run handlers,
stating that the CLI awaits the handler result rather than requiring every
handler to return a Promise. If the Promise requirement is retained, scope it
explicitly to the legacy plain-function hook API and keep defineHook compatible
with the synchronous examples.
Comment on lines
+125
to
+129
| const liveSyncResultInfo = { fake: true }; | ||
| await hooksService().executeAfterHooks("case2", { liveSyncResultInfo }); | ||
|
|
||
| assert.strictEqual(capture.payload.liveSyncResultInfo, liveSyncResultInfo); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert identity for the fallback payload.
The assertion only verifies that liveSyncResultInfo retains its identity. An implementation that shallow-copies the top-level argument still passes, but mutations to ctx.payload then do not affect the CLI object. Store the argument bag and assert capture.payload === payload.
Proposed test update
- const liveSyncResultInfo = { fake: true };
- await hooksService().executeAfterHooks("case2", { liveSyncResultInfo });
+ const liveSyncResultInfo = { fake: true };
+ const payload = { liveSyncResultInfo };
+ await hooksService().executeAfterHooks("case2", payload);
+ assert.strictEqual(capture.payload, payload);
assert.strictEqual(capture.payload.liveSyncResultInfo, liveSyncResultInfo);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const liveSyncResultInfo = { fake: true }; | |
| await hooksService().executeAfterHooks("case2", { liveSyncResultInfo }); | |
| assert.strictEqual(capture.payload.liveSyncResultInfo, liveSyncResultInfo); | |
| }); | |
| const liveSyncResultInfo = { fake: true }; | |
| const payload = { liveSyncResultInfo }; | |
| await hooksService().executeAfterHooks("case2", payload); | |
| assert.strictEqual(capture.payload, payload); | |
| assert.strictEqual(capture.payload.liveSyncResultInfo, liveSyncResultInfo); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/define-hook.ts` around lines 125 - 129, Update the test around
hooksService().executeAfterHooks to store the full argument bag in a payload
variable, pass that same variable to executeAfterHooks, and assert
capture.payload is strictly identical to payload. Replace the nested
liveSyncResultInfo identity assertion so shallow-copy implementations no longer
satisfy the test.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
What is the current behavior?
Hooks are plain functions whose shape the CLI infers at runtime: services arrive via parameter-name injection (deprecated, runtime-traced), the payload via a magic
hookArgsparameter, middleware by returning a function (implicit and undocumented), and aborting by throwing an error carryingstopExecution/errorAsWarningfields.What is the new behavior?
A typed, explicit hook-authoring API — fully additive; every existing hook keeps working unchanged.
lib/common/define-hook.tsis import-free (loading it can never boot a second CLI runtime) and re-exported fromnativescript/contracts; definitions carry a plain-assignedSymbol.formarker — spread-safe ({ ...definition }stays recognized) and recognizable across duplicated CLI copies in an extension tree.isHookDefinitionis a type predicate.defineHook<TPayload>/HookContext<TPayload>type the payload;ctx.payloadisTPayload | undefinedbecause dispatch-fired hook points carry no payload.nameorrun, unknown fields, and array exports all throw naming the definition and both accepted forms — no more deep unattributed failures.namediffers from the invoking hook point is skipped with a visible warning (so future manifest routing that honors names is not a behavior change).ctx.wrap()never silently no-ops: at hook points that don't consume middlewares (after-hooks,before-<command>dispatch,before-build-task-args,before-watchAction) it throws with a clear error; the wrappable set (the@hook-decorated before-points) is documented.projectDatapromotion hack, no deprecation report. Plain-function hooks are untouched..mjsdefault exports are recognized; a handler that returns a function gets a warning pointing atctx.wrap().wrap()middlewares feed the exact channel legacy returned-functions use, so they compose with the@hookdecorator chain identically. Per-directory hook results are flattened once so multi-middleware hooks surface correctly (returned arrays of functions were silently dropped before and were never in the documented contract).extending-cli.mdnow leads withdefineHook; plain-function hooks and parameter-name injection remain documented as the transitional and legacy tiers, with thestopExecutioncontract corrected and the watch hook-point names fixed.Public type names follow the new-API convention (no
Iprefix):HookContext,HookDefinition,HookMiddleware. Legacy publishedI*types are untouched.Full suite: 115 files, 1739 passed / 9 skipped (main baseline 1713 + 26 branch tests); the yok oracle, public-API test, and compat fixtures are untouched.
Summary by CodeRabbit
New Features
defineHookAPI for creating reusable CLI hooks.Documentation