port: fix issues w/ ExpectReplies, invoke in SkillDialog #2966
Conversation
joshgummersall
requested review from
carlosscastro,
johnataylor,
Stevenic and
a team
as code owners
| conversationId: string, | ||
| activity: Activity | ||
| ) => Promise<InvokeResponse>); | ||
| postActivity: <T = any>( |
There was a problem hiding this comment.
This is the fix to remove the unnecessary as casting here. The signatures are equivalent due as T is defaulted to any.
joshgummersall
changed the title
Also includes some small cleanup/formatting changes. Fixes #2962
joshgummersall
force-pushed
the
jpg/port-fix-issue-with-expect-replies-and-invoke-in-skill-dialog
branch
from
c94af39 to
ec889ba
Compare
|
|
||
| // Ensure the TurnState has the InvokeResponseKey, since this activity | ||
| // is not being sent through the adapter, where it would be added to TurnState. | ||
| if (a.type === ActivityTypes.InvokeResponse) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
The JS code looks good 👍
@gabog, @johnataylor do we need to use TryAdd in .NET or some sort of "TrySet"?
There was a problem hiding this comment.
We want to do a last-write wins in .NET if I'm correctly recall the implementation around TeamsActivityHandler. .Add() (depending on the class/implementation) normally throws an exception iirc.
There was a problem hiding this comment.
Looks good to me, @EricDahlvang, would you be able to take a quick pass and make sure nothing is missing?
There was a problem hiding this comment.
This looks fine... Are we squared away in .NET with test coverage? If not can you do a pass at testing the JS E2E with @gabog's assistance and make sure test coverage is added for these changes in JS?
(Either update the PR with some unit tests (preferable) or file an issue for R12 to add appropriate unit tests)
|
I've created #2969 so we don't hold up the new RC. |
joshgummersall
deleted the
jpg/port-fix-issue-with-expect-replies-and-invoke-in-skill-dialog
branch
Also includes some small cleanup/formatting changes. Fixes #2962
Also includes some small cleanup/formatting changes.
Fixes #2962