-
Notifications
You must be signed in to change notification settings - Fork 684
feat(nodejs): add typed event filtering to session.on() #272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds typed event filtering to the CopilotSession.on() method, enabling developers to subscribe to specific event types with full TypeScript type inference. The implementation maintains backward compatibility with the existing wildcard subscription pattern.
Changes:
- Added type definitions (
SessionEventType,SessionEventPayload,TypedSessionEventHandler) to support typed event subscriptions - Overloaded the
on()method to accept either a specific event type with typed handler, or a wildcard handler for all events - Updated documentation and examples to demonstrate the new typed event subscription pattern
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| nodejs/src/types.ts | Adds new type definitions for event type strings, payload extraction, and typed event handlers |
| nodejs/src/session.ts | Implements overloaded on() method and updates event dispatch logic to support typed handlers |
| nodejs/src/index.ts | Exports the new type definitions for public API usage |
| nodejs/README.md | Updates API documentation and examples to show typed event subscription patterns |
| docs/getting-started.md | Updates getting started examples to use the new typed event handler syntax |
Comments suppressed due to low confidence (3)
nodejs/src/session.ts:267
- If the handler parameter is undefined at runtime (which could occur through type assertions, JavaScript usage, or other bypasses of TypeScript's type system), the check on line 248 would fail, causing the code to fall through to line 263 where it would treat the string eventType as a SessionEventHandler. This would add a string to
eventHandlers, leading to a runtime error when _dispatchEvent tries to invoke it as a function.
Consider adding explicit validation: if (typeof eventTypeOrHandler === "string" && handler === undefined) { throw new Error("Handler is required when subscribing to specific event types"); } before the fallthrough to the wildcard handler logic. This provides a clearer error message for this edge case.
this.typedEventHandlers.set(eventType, new Set());
}
this.typedEventHandlers.get(eventType)!.add(handler);
return () => {
const handlers = this.typedEventHandlers.get(eventType);
if (handlers) {
handlers.delete(handler);
}
};
}
// Overload 2: on(handler) - wildcard subscription
const wildcardHandler = eventTypeOrHandler as SessionEventHandler;
this.eventHandlers.add(wildcardHandler);
return () => {
this.eventHandlers.delete(wildcardHandler);
};
}
/**
nodejs/src/session.ts:268
- The new typed event handler functionality (calling
on()with a specific event type) lacks test coverage. While tests exist for the wildcard handler pattern, there are no tests verifying:
- Typed handlers receive only events of the specified type
- Type inference works correctly for typed handlers
- The unsubscribe function works correctly for typed handlers
- Both typed and wildcard handlers can coexist and both receive events appropriately
Consider adding unit tests covering these scenarios to ensure the new feature works as expected.
): () => void {
// Overload 1: on(eventType, handler) - typed event subscription
if (typeof eventTypeOrHandler === "string" && handler) {
const eventType = eventTypeOrHandler;
if (!this.typedEventHandlers.has(eventType)) {
this.typedEventHandlers.set(eventType, new Set());
}
this.typedEventHandlers.get(eventType)!.add(handler);
return () => {
const handlers = this.typedEventHandlers.get(eventType);
if (handlers) {
handlers.delete(handler);
}
};
}
// Overload 2: on(handler) - wildcard subscription
const wildcardHandler = eventTypeOrHandler as SessionEventHandler;
this.eventHandlers.add(wildcardHandler);
return () => {
this.eventHandlers.delete(wildcardHandler);
};
}
/**
* Dispatches an event to all registered handlers.
nodejs/src/session.ts:282
- The type assertion
event as SessionEventPayload<typeof event.type>is not correctly narrowing the type. Thetypeof event.typeexpression evaluates tostringrather than the specific literal type (e.g., "assistant.message"). This means TypeScript won't properly narrow the event type for handlers.
Since the event is already the correct discriminated union type (SessionEvent), and you're dispatching it to handlers that were registered for event.type, the event should already be type-safe. The cast is unnecessary - you can simply pass event directly to the handler.
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nodejs/src/session.ts
Outdated
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| private typedEventHandlers: Map<SessionEventType, Set<(event: any) => void>> = new Map(); |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type for handler storage (Map<SessionEventType, Set<(event: any) => void>>) bypasses TypeScript's type safety. While this might seem necessary for storing handlers of different types in the same Map, it undermines the type safety that the typed event system is designed to provide.
Consider using a more type-safe approach, such as defining the Map as Map<string, Set<(event: SessionEvent) => void>> since the handlers will receive the full SessionEvent anyway (after the type cast issue on line 282 is fixed). This maintains type safety while still allowing storage of handlers for different event types.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| private typedEventHandlers: Map<SessionEventType, Set<(event: any) => void>> = new Map(); | |
| private typedEventHandlers: Map<SessionEventType, Set<(event: SessionEvent) => void>> = | |
| new Map(); |
Cross-SDK Consistency ReviewThis PR adds a valuable developer experience improvement to the Node.js SDK by introducing typed event filtering via an overloaded Summary✅ Node.js/TypeScript (after this PR):
❌ Python, Go, and .NET (current state):
ImpactThis enhancement significantly improves the Node.js developer experience by:
However, developers using Python, Go, or .NET will need to continue using the manual filtering pattern: # Python developers still need this pattern:
session.on(lambda event:
process_delta(event.data.delta_content)
if event.type == "assistant.message_delta"
else None
)RecommendationConsider implementing equivalent typed event filtering in the other SDKs to maintain feature parity: Python: Could use method overloading or a separate session.on("assistant.message", lambda event: print(event.data.content))Go: Could use a generic typed subscription method: copilot.On[copilot.AssistantMessageEvent](session, func(event copilot.AssistantMessageEvent) {
fmt.Println(event.Data.Content)
})C#/.NET: Could leverage event types for filtering: session.On(AssistantMessageEvent)(evt => Console.WriteLine(evt.Data?.Content));This is not a blocking issue, but tracking this gap will help maintain consistent developer experience across all SDK languages. Great feature for Node.js! 🎉
|
Add overloaded on() method that accepts an event type string as the first
argument, enabling type-safe event subscriptions:
session.on('assistant.message', (event) => {
// event is typed as SessionEventPayload<'assistant.message'>
console.log(event.data.content);
});
The original on(handler) signature remains supported for wildcard subscriptions.
Changes:
- Add SessionEventType, SessionEventPayload, TypedSessionEventHandler types
- Update CopilotSession.on() with typed overload
- Update _dispatchEvent() to dispatch to typed handlers
- Export new types from index.ts
- Update documentation with new usage patterns
5764124 to
f3514a0
Compare
Cross-SDK Consistency ReviewSummaryThis PR adds typed event filtering to the Node.js/TypeScript SDK, introducing an overloaded Current State Across SDKs✅ Node.js/TypeScript (This PR)After this PR: // New: Type-safe event filtering
session.on("assistant.message", (event) => {
console.log(event.data.content); // TypeScript knows event.data structure
});
// Existing: Wildcard subscription still supported
session.on((event) => {
if (event.type === "assistant.message") { ... }
});❌ Python SDKCurrent state: # Only supports wildcard subscription
def handler(event: SessionEvent) -> None:
if event.type == SessionEventType.ASSISTANT_MESSAGE:
print(event.data.content)
session.on(handler)Observation: Python SDK does NOT have typed event filtering. All event subscriptions receive all events and must manually filter by ❌ Go SDKCurrent state: // Only supports wildcard subscription
session.On(func(event copilot.SessionEvent) {
switch event.Type {
case copilot.AssistantMessage:
fmt.Println(event.Data.Content)
}
})Observation: Go SDK does NOT have typed event filtering. All handlers receive all events and use switch statements to filter. ❌ .NET SDKCurrent state: // Only supports wildcard subscription with pattern matching
session.On(evt =>
{
if (evt is AssistantMessageEvent assistantMessage)
{
Console.WriteLine(assistantMessage.Data?.Content);
}
});Observation: .NET SDK does NOT have typed event filtering. It uses C# pattern matching for event discrimination. Consistency Assessment🟡 Feature Parity Gap DetectedThis PR introduces a developer experience enhancement exclusive to the Node.js SDK. While the change is TypeScript-specific and leverages language features unavailable in other languages, the API design pattern could be adapted to other SDKs using their respective language idioms. Language-Appropriate AdaptationsEach language could implement similar event filtering with their native patterns: Python (suggested): # Option 1: Using literal strings (simpler)
session.on("assistant.message", lambda event: print(event.data.content))
# Option 2: Using enum values (more type-safe)
session.on(SessionEventType.ASSISTANT_MESSAGE, lambda event: print(event.data.content))Go (suggested): // Using constant strings
session.OnEvent(copilot.AssistantMessage, func(event copilot.SessionEvent) {
fmt.Println(event.Data.Content)
}).NET (suggested): // Using generic overload
session.On(AssistantMessageEvent)(evt =>
{
Console.WriteLine(evt.Data?.Content);
});
// Or using string literal
session.On("assistant.message", (AssistantMessageEvent evt) =>
{
Console.WriteLine(evt.Data?.Content);
});Recommendation✅ This PR is acceptable as-isThe changes are well-designed and maintain backward compatibility. The Node.js SDK is leveraging TypeScript's advanced type system features. 💡 Consider follow-up workFor feature parity and consistent developer experience across all SDKs, consider:
These enhancements would provide a more ergonomic API across all languages while respecting each language's idioms and type system capabilities. 📝 Documentation NoteThe getting-started guide has been updated to showcase the new typed event handlers, which is excellent. If other SDKs implement similar features, their documentation should be updated similarly to maintain consistency in the learning experience. Verdict: ✅ No blocking issues. This is a TypeScript-specific enhancement that improves developer experience. Consider adding equivalent functionality to other SDKs to maintain API design consistency (accounting for language differences).
|
Add overloaded on() method that accepts an event type string as the first argument, enabling type-safe event subscriptions:
The original on(handler) signature remains supported for wildcard subscriptions.
Changes: