WIP: Add type parameter of event subtype for app.message listeners #796 #871
Conversation
seratch
added
enhancement
Codecov Report
@@ Coverage Diff @@
## main #871 +/- ##
=======================================
Coverage 66.08% 66.08%
=======================================
Files 13 13
Lines 1200 1200
Branches 353 353
=======================================
Hits 793 793
Misses 338 338
Partials 69 69
Continue to review full report at Codecov.
|
| ); | ||
| expectType<void>( | ||
| // no subtype in the event payload | ||
| app.message<undefined>('foo', async ({ message }) => { |
There was a problem hiding this comment.
Setting undefined as the default can be a breaking changes to the existing apps.
| EventType extends string = string, | ||
| EventSubtype extends string | undefined = undefined | ||
| > { | ||
| payload: EventSubtype extends '*' ? EventFromType<EventType> : EventFromTypeAndSubtype<EventType, EventSubtype>; |
There was a problem hiding this comment.
To keep backward compatibility, we are unable to use undefined for subtype here.
| public message(...listeners: Middleware<SlackEventMiddlewareArgs<'message'>>[]): void; | ||
| public message(pattern: string | RegExp, ...listeners: Middleware<SlackEventMiddlewareArgs<'message'>>[]): void; | ||
| public message(...patternsOrMiddleware: (string | RegExp | Middleware<SlackEventMiddlewareArgs<'message'>>)[]): void { | ||
| public message<Subtype extends string | undefined = '*'>( |
There was a problem hiding this comment.
I don't think this asterisk string is cool but what else can be relevant here?
There was a problem hiding this comment.
Asterisk made sense to me while reading it over. 👍🏻
| @@ -4,8 +4,6 @@ import { App, MessageEvent, GenericMessageEvent, BotMessageEvent, MessageReplied | |||
| const app = new App({ token: 'TOKEN', signingSecret: 'Signing Secret' }); | |||
|
|
|||
| expectType<void>( | |||
| // TODO: Resolve the event type when having subtype in a listener constraint | |||
| // app.message({pattern: 'foo', subtype: 'message_replied'}, async ({ message }) => {}); | |||
There was a problem hiding this comment.
As I mentioned in this comment, app.message({pattern: 'foo', subtype: 'message_replied'} or something like that can be another option. I'm not sure if it's better than the type parameter though.
| public message(...listeners: Middleware<SlackEventMiddlewareArgs<'message'>>[]): void; | ||
| public message(pattern: string | RegExp, ...listeners: Middleware<SlackEventMiddlewareArgs<'message'>>[]): void; | ||
| public message(...patternsOrMiddleware: (string | RegExp | Middleware<SlackEventMiddlewareArgs<'message'>>)[]): void { | ||
| public message<Subtype extends string | undefined = '*'>( |
There was a problem hiding this comment.
Asterisk made sense to me while reading it over. 👍🏻
| ): void; | ||
| public message<Subtype extends string | undefined = '*'>( | ||
| ...patternsOrMiddleware: (string | RegExp | Middleware<SlackSubtypedEventMiddlewareArgs<'message', Subtype>>)[] | ||
| ): void { | ||
| const messageMiddleware = patternsOrMiddleware.map((patternOrMiddleware) => { |
There was a problem hiding this comment.
TODO: oops, I forgot to add subtype constraint to the middleware. will do later.
seratch
changed the title
|
converted to draft for safety |
|
As we are aiming to release v3.4.0 within a few days and this pull request needs more time, let me move this one to v3.5 milestone. |
|
Let me close this long-lived but incomplete PR. If someone would like to reuse the code changes in the future, go ahead with it! 🚀 |
Summary
This pull request shows a possible improvement for #796 use case - better support for message event subtypes in TypeScript. I'm still wondering if there may be a better solution than this one. I would love to get inputs and feedback from anyone who is interested in this topic.
Requirements (place an
xin each[ ])