feat: allow string event names #2618
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Waku event system to allow event listeners to be registered using string names instead of enum values, improving developer experience and API flexibility.
- Converts
WakuEventfrom an enum to a const object with type union for better string literal support - Fixes libp2p type compatibility issues in FilterCore and stream manager tests
- Updates documentation examples to demonstrate string-based event registration
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/interfaces/src/waku.ts | Converts WakuEvent enum to const object and updates interface definitions and documentation to use string literals |
| packages/core/src/lib/stream_manager/stream_manager.spec.ts | Fixes type casting for libp2p connection manager access in tests and removes trailing whitespace |
| packages/core/src/lib/filter/filter.ts | Updates import statements and converts onRequest method to arrow function property for proper libp2p StreamHandler typing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| public async start(): Promise<void> { | ||
| try { | ||
| await this.libp2p.handle(FilterCodecs.PUSH, this.onRequest.bind(this), { | ||
| await this.libp2p.handle(FilterCodecs.PUSH, this.onRequest, { |
There was a problem hiding this comment.
Passing this.onRequest directly will lose the this context when the method is called. Since onRequest is now an arrow function property, this change is correct, but the arrow function should be defined before this line to ensure proper initialization order.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
What it says. I doubt you can remove the bind here without breaking things. this.onRequest uses this.handleIncomingMessage which I believe will not be available if not binded
| public async start(): Promise<void> { | ||
| try { | ||
| await this.libp2p.handle(FilterCodecs.PUSH, this.onRequest.bind(this), { | ||
| await this.libp2p.handle(FilterCodecs.PUSH, this.onRequest, { |
There was a problem hiding this comment.
What it says. I doubt you can remove the bind here without breaking things. this.onRequest uses this.handleIncomingMessage which I believe will not be available if not binded
| ]; | ||
|
|
||
| streamManager["libp2p"]["connectionManager"]["getConnections"] = ( | ||
| (streamManager["libp2p"]["connectionManager"] as any).getConnections = ( |
There was a problem hiding this comment.
can we avoid the as any in test? it just makes refactoring harder as your bypass the type system. Just leave it as it was.
| export const WakuEvent = { | ||
| Connection: "waku:connection", | ||
| Health: "waku:health" | ||
| } as const; |
There was a problem hiding this comment.
I thought you wanted to remove the enum? In any case, add a comment for next implementer to say to leave it as is to expose string for event types to developer.
2 participants
Summary
Testing
npm run buildnpm run checknpm run test:node --workspace packages/sdkhttps://chatgpt.com/codex/tasks/task_e_68b8c8b38f3c833194e44d778110c784