-
Notifications
You must be signed in to change notification settings - Fork 25
Introduce HttpServer (and begin deprecating HttpPlugin) #433
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: aamirj/simlifyHttp
Are you sure you want to change the base?
Conversation
f07e077 to
d9a529e
Compare
6601a42 to
7e55c15
Compare
| }); | ||
|
|
||
| expressApp.get('/', (req, res) => { | ||
| res.send(` |
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.
this is cute, should we do the same in other languages/samples?
2fa2032 to
4590e50
Compare
4c1ca04 to
5422e29
Compare
heyitsaamir
left a comment
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.
Questions:
- Should we remove app.initialize()? I feel like we could remove it, but i like having it because it could give us a way to initialize things asynchronously if needed.
- Ideally we should not be shipping with a server at all -- If we want to do
serve(new App())whereservecomes from anexpressspecific package, that'll lead to a pretty big breaking change for all our docs for our simplest use-cases. For this reason, I think we should leaveapp.startthe way it is, and for v3 consider removing.start.
| } | ||
| } | ||
|
|
||
| // initialize server |
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.
We could technically move this to the constructor, as with above ^. So then we won't need app.initialize() anymore.
|
|
||
| @Dependency() | ||
| protected readonly _httpPlugin!: HttpPlugin; | ||
| @HttpServer() |
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.
For these plugins, we are exposing the HttpServer because of backward compat. The A2A Plugin does a check to see if the adapter is Express or not since it was built with an express server in mind.
|
|
||
| @Dependency() | ||
| readonly httpPlugin!: HttpPlugin; | ||
| @HttpServer() |
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.
For these plugins, we are exposing the HttpServer because of backward compat. The MCP Plugin does a check to see if the adapter is Express or not since it was built with an express server in mind.
| * - Request/response data extraction and sending | ||
| * - Server lifecycle management | ||
| */ | ||
| export class ExpressAdapter implements IHttpAdapter { |
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.
This adapter does a lot, but that's mainly because we are maintaining some of the exposed functions from HttpPlugin.
| * - Request/response data extraction and sending | ||
| * - Server lifecycle management | ||
| */ | ||
| export class ExpressAdapter implements IHttpAdapter { |
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.
This implementation is the "Default" one we ship with. Ideally we should not be shipping with a server at all -- If we want to do serve(new App()) where serve comes from an express specific package, that'll lead to a pretty big breaking change for all our docs for our simplest use-cases. For this reason, I think we should leave app.start the way it is, and for v3 consider removing .start.
| } | ||
|
|
||
| try { | ||
| token = await this.validateJwt(authHeader, body); |
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.
This isn't using the middleware approach to validate tokens. Just doing it inline. I think that's okay, but if anyone thinks differently, lmk.
| name: 'http', | ||
| version: pkg.version, | ||
| description: 'the default plugin for receiving activities via HTTP', | ||
| description: 'Will be deprecated: Use HttpServer with server option instead', |
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.
Specifies that this is scheduled for deprecation.
| this.expressAdapter = new ExpressAdapter(server); | ||
| this._server = new HttpServer(this.expressAdapter, options); |
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.
Uses ExpressAdapter internally, since that was the behavior from before
30af118 to
4ec6679
Compare
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
4ec6679 to
95d91e9
Compare
In this PR, we introduce a new object called HttpServer and begin to deprecate HttpPlugin.
Main changes
HttpServerinternal class. It accepts anIHttpAdapterwhich is the server implementation.IHttpAdapterWhy:
HTTP is a core part of our sdk. Our App object uses HTTP to set up a server, perform auth validations, and pipe the request to the handlers that are attached, and then return the response. Key part is that Http is a core part of App, not a plugin, since core functionality is dependent on it.
Even inside the App object, we were doing special casing for this Http"Plugin" whereas it should never have really been a plugin to begin with. By making it a plugin, we were exposing many non-plugin essential things to the plugin system in general.
So what should it have been? Well, HTTP Plugin had these responsibilities
So, we introduce a new object called
HttpServerwhose responsibilities are essentially that ^. This object is not a plugin, but an object that's created by App itself.Customization
Now this idealogical shift doesn't really warrant us doing this refactor, but we started seeing requests from folks who wanted to hook Teams functionality into existing servers, or replace the underlying server infra with a non-express server. Our recommendation was to rebuild a new HttpPlugin. But rebuilding this plugin is not simple (since we don't really document it anywhere, and didn't expect folks to build their own).
So
HttpServerexposes anHttpAdapterconcept. To build the adapter, one simply needs to build out a handler for extracting request data, and a handler for responses. This means that you can build simple custom adapters for your own existing servers. (And if you don't pass one in, we'll build a default express one.) Examples of servers are in the http-adapters folder under examples/.Backward Compat
We've updated
HttpPluginto basically useHttpServerwith anExpressAdapterinternally for backward compat. I don't think this should lead to any breaking changes (even if someone passes in their ownHttpPlugin). (Tested BotBuilderPlugin, from examples, and it worked without any changes).However, it should be noted that I marked HttpPlugin as deprecated in this PR, so it should be discouraged going forward, and after the next few versions, it'll be removed.
Testing
I tested by running the following examples:
skip-test-verification (added manifest for tabs)
PR Dependency Tree
This tree was auto-generated by Charcoal