Skip to content

feat: add clearHook(name)#135

Merged
pi0 merged 2 commits into
mainfrom
feat/clear-hook
Mar 14, 2026
Merged

feat: add clearHook(name)#135
pi0 merged 2 commits into
mainfrom
feat/clear-hook

Conversation

@danielroe

@danielroe danielroe commented Mar 14, 2026

Copy link
Copy Markdown
Member

the aim is to allow handlers to be unregistered without requiring the original function references

(I was investigating ways of reducing memory usage in nuxt and thought that unregistering hooks might be useful - and in fact it did save some memory where those hooks closed over other variables, etc. - so thought it might be worth raising here...)

would also be happy to update removeHook to make the second argument optional if you prefer

... and of course, feel free to close if this isn't useful

Summary by CodeRabbit

  • New Features

    • Added the ability to explicitly clear registered hooks by name, improving hook lifecycle and cleanup control.
  • Tests

    • Added test coverage for hook-clearing behavior, including safe handling when clearing non-existent hooks and ensuring other hooks remain unaffected.

@danielroe danielroe requested a review from pi0 March 14, 2026 23:02
@coderabbitai

coderabbitai Bot commented Mar 14, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 376a826a-978c-4742-9813-08b8388342bc

📥 Commits

Reviewing files that changed from the base of the PR and between a3e1353 and 4a3d72c.

📒 Files selected for processing (1)
  • src/hookable.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/hookable.ts

📝 Walkthrough

Walkthrough

The PR adds a public method clearHook<NameT extends HookNameT>(name: NameT): void to the Hookable class in src/hookable.ts that sets this._hooks[name] = undefined. New tests in test/hookable.test.ts verify clearing behavior and safety when called for non-existent hooks.

Changes

Cohort / File(s) Summary
Hookable implementation
src/hookable.ts
Added clearHook<NameT extends HookNameT>(name: NameT): void to class Hookable; implementation sets the specified hook slot to undefined.
Hookable tests
test/hookable.test.ts
Added tests verifying that clearHook removes all handlers for a specified hook, preserves other hooks, and is safe when called for non-existent hooks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐇 I nudge the hooks and give a cheer,
One little call, and they disappear,
Undefined now where handlers stood,
Clean and tidy — isn't that good? ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add clearHook(name)' directly describes the main change—a new public method being added to the Hookable class.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/clear-hook
📝 Coding Plan
  • Generate coding plan for human review comments

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

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/hookable.test.ts (1)

315-318: Add a parity test for HookableCore.clearHook as well.

Line 315 validates non-existent hook safety on Hookable, but the same new API was also added to HookableCore. A small parity test would prevent regressions in that path.

Proposed test addition
 describe("HookableCore", () => {
   test("basic functionality", async () => {
@@
     expect(obj.called).toBe(true);
   });
+
+  test("clearHook removes only the target hook handlers", async () => {
+    const hookable = new HookableCore<{ a(): void; b(): void }>();
+    const a = vi.fn();
+    const b = vi.fn();
+
+    hookable.hook("a", a);
+    hookable.hook("a", a);
+    hookable.hook("b", b);
+
+    hookable.clearHook("a");
+
+    await hookable.callHook("a");
+    await hookable.callHook("b");
+
+    expect(a).toBeCalledTimes(0);
+    expect(b).toBeCalledTimes(1);
+  });
 });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/hookable.test.ts` around lines 315 - 318, Add a parity test for
HookableCore.clearHook mirroring the existing Hookable test: instantiate
HookableCore, call clearHook("test:nonexistent") and assert it does not throw.
Target the HookableCore class and its clearHook method (HookableCore.clearHook)
so the new test ensures the same non-existent-hook safety behavior as the
Hookable test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/hookable.test.ts`:
- Around line 315-318: Add a parity test for HookableCore.clearHook mirroring
the existing Hookable test: instantiate HookableCore, call
clearHook("test:nonexistent") and assert it does not throw. Target the
HookableCore class and its clearHook method (HookableCore.clearHook) so the new
test ensures the same non-existent-hook safety behavior as the Hookable test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: feab32fd-2ada-4ac1-b2e8-bf51416f4ffb

📥 Commits

Reviewing files that changed from the base of the PR and between 28f9af2 and a3e1353.

📒 Files selected for processing (2)
  • src/hookable.ts
  • test/hookable.test.ts

@pi0

pi0 commented Mar 14, 2026

Copy link
Copy Markdown
Member

Maybe we can only add it to non core version?

@pi0 pi0 merged commit 269dd02 into main Mar 14, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants