-
Notifications
You must be signed in to change notification settings - Fork 390
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 #904 by improving app.message listener's TS compatibility while bringing breaking changes #1801
base: main
Are you sure you want to change the base?
Changes from 1 commit
7df0382
16c2c3e
bc7af32
5dce168
c8f877a
b668d0b
22b5179
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,9 +50,9 @@ import { | |
InteractiveAction, | ||
ViewOutput, | ||
KnownOptionsPayloadFromType, | ||
KnownEventFromType, | ||
SlashCommand, | ||
WorkflowStepEdit, | ||
KnownEventFromType, | ||
} from './types'; | ||
import { IncomingEventType, getTypeAndConversation, assertNever } from './helpers'; | ||
import { CodedError, asCodedError, AppInitializationError, MultipleListenerError, ErrorCode, InvalidCustomPropertyError } from './errors'; | ||
|
@@ -182,9 +182,17 @@ export interface AnyErrorHandler extends ErrorHandler, ExtendedErrorHandler { | |
} | ||
|
||
// Used only in this file | ||
type MessageEventMiddleware< | ||
type AllMessageEventMiddleware< | ||
CustomContext extends StringIndexed = StringIndexed, | ||
> = Middleware<SlackEventMiddlewareArgs<'message', string | undefined>, CustomContext>; | ||
|
||
// Used only in this file | ||
type FilteredMessageEventMiddleware< | ||
CustomContext extends StringIndexed = StringIndexed, | ||
> = Middleware<SlackEventMiddlewareArgs<'message'>, CustomContext>; | ||
> = Middleware<SlackEventMiddlewareArgs< | ||
'message', | ||
undefined | 'bot_message' | 'file_share' | 'thread_broadcast' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it is worth extracting this union into its own type? |
||
>, CustomContext>; | ||
|
||
class WebClientPool { | ||
private pool: { [token: string]: WebClient } = {}; | ||
|
@@ -535,21 +543,27 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
eventName: EventType, | ||
...listeners: Middleware<SlackEventMiddlewareArgs<EventType>, AppCustomContext & MiddlewareCustomContext>[] | ||
...listeners: Middleware< | ||
SlackEventMiddlewareArgs, | ||
AppCustomContext & MiddlewareCustomContext | ||
>[] | ||
): void; | ||
public event< | ||
EventType extends RegExp = RegExp, | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
eventName: EventType, | ||
...listeners: Middleware<SlackEventMiddlewareArgs<string>, AppCustomContext & MiddlewareCustomContext>[] | ||
...listeners: Middleware<SlackEventMiddlewareArgs, AppCustomContext & MiddlewareCustomContext>[] | ||
): void; | ||
public event< | ||
EventType extends EventTypePattern = EventTypePattern, | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
eventNameOrPattern: EventType, | ||
...listeners: Middleware<SlackEventMiddlewareArgs<string>, AppCustomContext & MiddlewareCustomContext>[] | ||
...listeners: Middleware< | ||
SlackEventMiddlewareArgs, | ||
AppCustomContext & MiddlewareCustomContext | ||
>[] | ||
): void { | ||
let invalidEventName = false; | ||
if (typeof eventNameOrPattern === 'string') { | ||
|
@@ -581,7 +595,7 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |
*/ | ||
public message< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>(...listeners: MessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[]): void; | ||
>(...listeners: FilteredMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[]): void; | ||
/** | ||
* | ||
* @param pattern Used for filtering out messages that don't match. | ||
|
@@ -592,7 +606,7 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
pattern: string | RegExp, | ||
...listeners: MessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
...listeners: FilteredMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
): void; | ||
/** | ||
* | ||
|
@@ -605,9 +619,9 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |
public message< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
filter: MessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>, | ||
filter: FilteredMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>, | ||
pattern: string | RegExp, | ||
...listeners: MessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
...listeners: FilteredMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
): void; | ||
/** | ||
* | ||
|
@@ -618,8 +632,8 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |
public message< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
filter: MessageEventMiddleware, | ||
...listeners: MessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
filter: FilteredMessageEventMiddleware, | ||
...listeners: FilteredMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
): void; | ||
/** | ||
* This allows for further control of the filtering and response logic. Patterns and middlewares are processed in | ||
|
@@ -630,16 +644,78 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |
public message< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
...patternsOrMiddleware: (string | RegExp | MessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>)[] | ||
...patternsOrMiddleware: ( | ||
| string | ||
| RegExp | ||
| FilteredMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>)[] | ||
): void; | ||
public message< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
...patternsOrMiddleware: (string | RegExp | MessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>)[] | ||
...patternsOrMiddleware: ( | ||
| string | ||
| RegExp | ||
| FilteredMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>)[] | ||
): void { | ||
const messageMiddleware = patternsOrMiddleware.map((patternOrMiddleware) => { | ||
if (typeof patternOrMiddleware === 'string' || util.types.isRegExp(patternOrMiddleware)) { | ||
return matchMessage<undefined | 'bot_message' | 'file_share' | 'thread_broadcast'>(patternOrMiddleware, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If my comment above is followed (to extract this union into its own type), we can then re-use it here. |
||
} | ||
return patternOrMiddleware; | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
}) as any; // FIXME: workaround for TypeScript 4.7 breaking changes | ||
|
||
this.listeners.push([ | ||
onlyEvents, | ||
matchEventType('message'), | ||
...messageMiddleware, | ||
] as Middleware<AnyMiddlewareArgs>[]); | ||
} | ||
|
||
public allMessageSubtypes< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>(...listeners: AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[]): void; | ||
public allMessageSubtypes< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
pattern: string | RegExp, | ||
...listeners: AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
): void; | ||
public allMessageSubtypes< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
filter: AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>, | ||
pattern: string | RegExp, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to clarify, |
||
...listeners: AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
): void; | ||
public allMessageSubtypes< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
filter: AllMessageEventMiddleware, | ||
...listeners: AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
): void; | ||
public allMessageSubtypes< | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this overload signature the same as the implementation signature that follows, or am I misreading this? |
||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
...patternsOrMiddleware: ( | ||
| string | ||
| RegExp | ||
| AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>)[] | ||
): void; | ||
/** | ||
* Accepts all subtype events of message ones. | ||
*/ | ||
public allMessageSubtypes< | ||
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
...patternsOrMiddleware: ( | ||
| string | ||
| RegExp | ||
| AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>)[] | ||
): void { | ||
const messageMiddleware = patternsOrMiddleware.map((patternOrMiddleware) => { | ||
if (typeof patternOrMiddleware === 'string' || util.types.isRegExp(patternOrMiddleware)) { | ||
return matchMessage(patternOrMiddleware); | ||
return matchMessage<string | undefined | never>(patternOrMiddleware, false); | ||
} | ||
return patternOrMiddleware; | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
|
@@ -943,8 +1019,15 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |
|
||
// Set body and payload | ||
// TODO: this value should eventually conform to AnyMiddlewareArgs | ||
let payload: DialogSubmitAction | WorkflowStepEdit | SlackShortcut | KnownEventFromType<string> | SlashCommand | ||
| KnownOptionsPayloadFromType<string> | BlockElementAction | ViewOutput | InteractiveAction; | ||
let payload: DialogSubmitAction | ||
| WorkflowStepEdit | ||
| SlackShortcut | ||
| KnownEventFromType<string, string | undefined> | ||
| SlashCommand | ||
| KnownOptionsPayloadFromType<string> | ||
| BlockElementAction | ||
| ViewOutput | ||
| InteractiveAction; | ||
switch (type) { | ||
case IncomingEventType.Event: | ||
payload = (bodyArg as SlackEventMiddlewareArgs['body']).event; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,18 +206,29 @@ export function matchConstraints( | |
}; | ||
} | ||
|
||
const messagePostedEventSubtypesAsArray = [undefined, 'bot_message', 'file_share', 'thread_broadcast']; | ||
|
||
/* | ||
* Middleware that filters out messages that don't match pattern | ||
*/ | ||
export function matchMessage( | ||
export function matchMessage< | ||
Subtypes extends string | undefined = string | undefined, | ||
>( | ||
pattern: string | RegExp, | ||
): Middleware<SlackEventMiddlewareArgs<'message' | 'app_mention'>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. having app_mention here had been false as a preceding built-in middleware There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK so this change implies that |
||
onlyMessagePosted: boolean = false, // false for backward compatibility | ||
): Middleware<SlackEventMiddlewareArgs<'message', Subtypes>> { | ||
return async ({ event, context, next }) => { | ||
let tempMatches: RegExpMatchArray | null; | ||
|
||
if (!('text' in event) || event.text === undefined) { | ||
return; | ||
} | ||
// Since version 3.14, handling only message posted events are allowed | ||
if (onlyMessagePosted && | ||
event.type === 'message' && | ||
!messagePostedEventSubtypesAsArray.includes(event.subtype)) { | ||
return; | ||
} | ||
|
||
// Filter out messages or app mentions that don't contain the pattern | ||
if (typeof pattern === '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.
Just an idea: many of the
assert
methods take a type parameter that then ensures the type of the argument provided toassert
is of the provided type. This way perhaps it is clearer the intention of the test ("verify it compiles").