feat: add clearHook(name)#135
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a public method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/hookable.test.ts (1)
315-318: Add a parity test forHookableCore.clearHookas well.
Line 315validates non-existent hook safety onHookable, but the same new API was also added toHookableCore. 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
📒 Files selected for processing (2)
src/hookable.tstest/hookable.test.ts
|
Maybe we can only add it to non core version? |
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
removeHookto 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
Tests