Simplify setting http response after activity processing #398
Conversation
heyitsaamir
left a comment
heyitsaamir
left a comment
There was a problem hiding this comment.
Makes sense to me. Add tests?
| ): Promise<InvokeResponse> { | ||
| this.events.emit('activity', event); | ||
| await this.process(sender, { ...event, sender }); | ||
| return await this.process(sender, { ...event, sender }); |
There was a problem hiding this comment.
Looks good to me.
One thing I think we should do is have an onActivity in apps (core, not app.events). That should call emit's onActivity, and then call process from there.
There was a problem hiding this comment.
One issue with that is that other event handler methods will be here... do we want to have that discrepancy? Let's plan that refactor for later?
There was a problem hiding this comment.
Right, but now process is no longer tied to an event right?
There was a problem hiding this comment.
teams.ts/packages/apps/src/app.ts
Line 507 in ee110ba
the process is not really tied to an event (it's tied to the event manager) and the App's
OnActivity implementation is just ^^.
There was a problem hiding this comment.
right that's what i mean. Why does onActivity needs to be in events anymore? It can be in the main app file. The flow of data is weird right now, app.events is powering onActivity for no clear reason
There was a problem hiding this comment.
It's because the responsibility of the app.events is to handle events, and OnActivity is one of them. Moving it to app will split the responsibility of managing events between app and app.events.
There was a problem hiding this comment.
it's responsible for emitting events, not really handling them.
"process" is part of handling an event that's happening. I think what makes sense is:
Http plugin gets the http event -> forwards the event to app -> app sends the event to be processed -> emits it out/
wdyt?
Linked PR: microsoft/teams.ts#398 This is how the architecture looked like before <img width="1131" height="573" alt="image" src="https://github.com/user-attachments/assets/f24e60a3-8326-497d-b63b-e0a3e2579c3f" /> This is what it looks like now: <img width="1168" height="556" alt="image" src="https://github.com/user-attachments/assets/450c7092-3595-4bfe-89ae-f3f1ff875aba" /> --------- Co-authored-by: heyitsaamir <[email protected]>
Ensure that HTTP responses are returned directly from handlers, rather than through event-based callbacks.
Activity processing and response flow:
onActivityhandler inapp.events.tsnow returns aPromise<InvokeResponse>and directly returns the result of processing the activity, rather than just awaiting it.$processfunction inapp.process.tsis refactored to always return anInvokeResponse, handling both normal and error cases, and ensuring response construction is consistent.HttpPlugin interface and implementation updates:
$onActivityevent handler inHttpPluginis now asynchronous and returns aPromise<InvokeResponse>, aligning with the new app contract.HttpPluginnow awaits the activity response and sends it directly to the client, removing the previous pattern of storing pending responses and using callback events.onError,onActivityResponse) and related state management are removed fromHttpPlugin, as response handling is now synchronous with the HTTP request.Type and import consistency:
InvokeResponsetype is imported and used consistently across files to improve type safety and clarity.