-
Notifications
You must be signed in to change notification settings - Fork 25
Simplify HTTP Plugin architecture #424
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
base: main
Are you sure you want to change the base?
Conversation
f07e077 to
d9a529e
Compare
| * Activity type for basic routing | ||
| */ | ||
| sender: ISender; | ||
| type?: string; |
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.
instead of string, should we be more specific here? or wait till it passes into app.process
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.
yeah, similar to the core in dotnet, this layer should be as unopinionated as possible. So ideally it doesn't really even know about Activity
The previous architecture tightly coupled HTTP transport concerns with activity sending logic:
**Previous Architecture:**
```
HttpPlugin (transport) → implements ISender (sending)
→ has send() method (creates new Client per call)
→ has createStream() method
→ knows about Activity protocol details
ActivityContext → depends on ISender plugin
→ cannot work without transport plugin
→ conflates transport and sending concerns
```
Key Issues:
- HttpPlugin created NEW Client instances on every send() call
- Transport plugins (HttpPlugin) were forced to implement send/createStream
- Users couldn't "bring their own server" without implementing ISender
- ActivityContext was tightly coupled to plugin architecture
- HttpPlugin knew about Bot Framework Activity protocol details
```
HttpPlugin (transport) → only handles HTTP server/routing/auth
→ emits ICoreActivity (minimal protocol knowledge)
→ just passes body payload to app
ActivitySender (NEW) → dedicated class for sending activities
→ receives injected, reusable Client
→ handles all send/stream logic
→ private to App class
ActivityContext → uses send callback (abstraction)
→ receives pre-created stream
→ no direct dependency on ActivitySender
```
- Centralized all activity sending logic
- Receives reusable Client in constructor (no per-send instantiation)
- Private to App class - internal implementation detail
- Provides send() and createStream() methods
- Minimal fields transport layer needs: serviceUrl, id, type
- Extensible via [key: string]: any for protocol-specific fields
- Transport plugins work with this instead of full Activity type
- Parsing to Activity happens in app.process.ts
- No longer needed - plugins don't send activities
- Plugins only handle transport (receiving requests)
- Breaking change, but simplifies plugin architecture
- Constructor accepts send callback function
- Receives pre-created stream (not factory function)
- No knowledge of ActivitySender implementation
- Proper abstraction via dependency injection
- Initially renamed to ActivityStream
- Reverted because it's still HTTP-specific (uses Bot Framework HTTP Client API)
- Moved from src/plugins/http/stream.ts to src/http-stream.ts
- Still transport-specific, just not plugin-owned
1. **ISender removed** - Custom plugins should implement IPlugin only
2. **IActivityEvent changed** - Now has body: ICoreActivity instead of activity: Activity
3. **Plugin.onActivity** - Still receives parsed activity: Activity (unchanged)
4. **App.process signature** - Internal change, not exposed to plugin API
Before:
```typescript
class MyPlugin implements ISender {
send(activity, ref) { ... } // Required
createStream(ref) { ... } // Required
async onRequest(req, res) {
const activity: Activity = req.body; // Need Activity type
await this.$onActivity({ activity, token });
}
}
```
After:
```typescript
class MyPlugin implements IPlugin {
// No send() or createStream() needed!
async onRequest(req, res) {
// Just pass body - don't need Activity type knowledge
await this.$onActivity({
body: req.body, // ICoreActivity (minimal fields)
token
});
}
}
```
1. **Client Reuse** - ActivitySender reuses same Client, no per-send instantiation
2. **Separation of Concerns** - Transport vs sending clearly separated
3. **Bring Your Own Server** - Easy to implement custom transports (Socket.io, gRPC, etc.)
4. **Less Protocol Knowledge** - Transport layer only needs ICoreActivity, not full Activity
5. **Cleaner Architecture** - Each class has single responsibility
6. **Better Abstraction** - ActivityContext uses callbacks, not direct dependencies
- src/activity-sender.ts - Dedicated activity sending class
- src/http-stream.ts - Moved from src/plugins/http/stream.ts
- src/app.ts - Added activitySender, updated send()
- src/app.process.ts - Removed sender param, uses activitySender
- src/plugins/http/plugin.ts - Removed send/createStream, works with ICoreActivity
- src/contexts/activity.ts - Uses callbacks instead of ISender plugin
- src/events/activity.ts - Added ICoreActivity, changed to body field
- src/types/plugin/sender.ts - Removed ISender, kept IActivitySender
- src/plugins/http/stream.ts - Moved to src/http-stream.ts
- ISender interface - Completely removed
- Add validation for missing activity body in app.process.ts - Add try-catch in HttpPlugin.onRequest to prevent server hanging on errors - Fix DevtoolsPlugin to properly handle promise rejections - Add default type fallback in devtools activity creation route Fixes issue where server would become unresponsive after activity processing errors. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
2fa2032 to
4590e50
Compare
Separate activity sending from HTTP transport layer
The previous architecture tightly coupled HTTP transport concerns with activity sending logic:
Previous Architecture:
There are a few issues with this:
New Architecture
In this PR, I am mainly decoupling responsibilities of HttpPlugin from being BOTH a listener AND a sender, to being just a listener. The sender bit is now separated to a different
ActivitySenderclass. Other than better code organization, the main thing this lets us do is not require the app to run to be able to send proactive messages. This is a huge plus point because now the App can be used in scenarios where it doesn't necessarily need to listen to incoming messages (like agentic notifications!)Major Decisions
1. Created ActivitySender Class
2. Introduced ICoreActivity Interface
3. Removed ISender Interface
Breaking Changes
For Plugin Authors:
PR Dependency Tree
This tree was auto-generated by Charcoal