-
Notifications
You must be signed in to change notification settings - Fork 398
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
fix: auto acknowledge self events before ignoring #2329
base: autoack
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## autoack #2329 +/- ##
========================================
Coverage 91.08% 91.09%
========================================
Files 22 22
Lines 6116 6122 +6
Branches 655 655
========================================
+ Hits 5571 5577 +6
Misses 540 540
Partials 5 5 ☔ View full report in Codecov by Sentry. |
Does this address the current non-acknowledged events in the Assistant class, or unrelated? |
@misscoded this should be unrelated |
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.
I have a concern about the test utilities, which is likely a symptom of a deeper issue relating to TS union types.
@@ -59,13 +59,14 @@ const ack: AckFn<void> = (_r?) => Promise.resolve(); | |||
export function wrapMiddleware<Args extends AnyMiddlewareArgs>( | |||
args: Args, | |||
ctx?: Context, | |||
): Args & AllMiddlewareArgs & { next: SinonSpy } { | |||
): Args & AllMiddlewareArgs & { next: SinonSpy; ack: SinonSpy } { |
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.
I don't think this is correct. Args
here extends AnyMiddlewareArgs
which is a union of different kinds of middleware args. Some of these middleware args contain ack
and others do not, so in theory Args
should be carrying the correct type, including the conditional presence of ack
.
You may have to narrow the return type from this method appropriately to get the right kind of Args
depending on the situation, which should in theory sometimes carry ack
.
I assume the issue was asserting on ack
in the test file you changed in this PR?
Summary
Since #2283 event requests are no longer auto acknowledged in the
processEvent
function. This behavior was handed over to theautoAcknowledge
middleware. This created a gap in theignoreSelf
middleware since the events reaching it were assumed to already be acknowledged. This PR aims to add auto acknowledge behavior into theignoreSelf
global middleware.Requirements (place an
x
in each[ ]
)