-
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?
Conversation
… while bringing breaking changes
pattern: string | RegExp, | ||
): Middleware<SlackEventMiddlewareArgs<'message' | 'app_mention'>> { |
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.
having app_mention here had been false as a preceding built-in middleware matchEventType('message')
does not accept app_mention. Moreover, it should not accept two types of events. So, I've removed it.
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.
OK so this change implies that matchMessage
should only ever be used with a message
event - might be useful to clarify that in the JSDoc, as the JSDoc in its current form implies this can be used for any event that contains a message.
Coverted to draft as it seems that this PR is not yet complete. |
Resolved the issues with |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1801 +/- ##
==========================================
+ Coverage 81.97% 82.07% +0.10%
==========================================
Files 18 18
Lines 1531 1540 +9
Branches 440 443 +3
==========================================
+ Hits 1255 1264 +9
Misses 178 178
Partials 98 98 ☔ View full report in Codecov by Sentry. |
type MessageMiddlewareArgs = SlackEventMiddlewareArgs<'message'> & MiddlewareCommonArgs; | ||
type MessageMiddlewareArgs = SlackEventMiddlewareArgs< | ||
'message', | ||
undefined | 'bot_message' | 'file_share' | 'thread_broadcast' |
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.
this aligns with bolt-python
Wow, this is sweet! These changes seem really helpful for improving the experience with TypeScript, particularly with the first The But regardless of that last point (it's only a suggestion that you can totally ignore!) I think this is a great proposal and would love to see it in an upcoming release! I agree that because this can be a breaking change, it may be better for a major release, but also understand releasing it in a minor version to bring these improvements to the near-term. The minor release would just require good communication about any breakages. However, if there are more breaking changes on the horizon, then planning for a major release soon would make more sense to me. |
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.
Looks good to me! Too bad about the breaking change but I am not sure if there is another option.
I wonder if it would be helpful to meet or collaborate to understand what other (if any) pending breaking changes we could address?
> = 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is worth extracting this union into its own type?
What does this combination represent? The string literals seem to be message subtypes, but undefined
in this context represents what? Generic message event? Message event with no subtype?
Just trying to better understand the intention.
): 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 comment
The 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.
MiddlewareCustomContext extends StringIndexed = StringIndexed, | ||
>( | ||
filter: AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>, | ||
pattern: string | RegExp, |
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 to clarify, filter
in these arguments means a middleware filter, whereas pattern
is a message text pattern match / filter?
filter: AllMessageEventMiddleware, | ||
...listeners: AllMessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[] | ||
): void; | ||
public allMessageSubtypes< |
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.
Is this overload signature the same as the implementation signature that follows, or am I misreading this?
pattern: string | RegExp, | ||
): Middleware<SlackEventMiddlewareArgs<'message' | 'app_mention'>> { |
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.
OK so this change implies that matchMessage
should only ever be used with a message
event - might be useful to clarify that in the JSDoc, as the JSDoc in its current form implies this can be used for any event that contains a message.
expectType<MessagePostedEvent>(message); | ||
|
||
message.channel; // the property access should compile | ||
message.user; // the property access should compile |
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.
Nice!
it('should provide better typed payloads', async () => { | ||
app.message(async ({ payload }) => { | ||
// verify it compiles | ||
assert.isNotNull(payload.channel); |
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 to assert
is of the provided type. This way perhaps it is clearer the intention of the test ("verify it compiles").
assert.isNotNull(payload.channel); | |
assert.isNotNull<string>(payload.channel); |
Summary
As mentioned in #904, implementing
app.message
listeners in TypeScript can be confusing as thepayload
/message
objects are union types, so developers must have if/else statements to determine the message payload. In most cases, developers just want to handle newly posted messages in the listener. Having to addif (!payload.subtype || payload.subtype === 'bot_message' || payload.subtype === 'file_share' || payload.subtype === 'thread_broadcast') { ... }
in allapp.message
listeners is not a great developer experience.I propose improving the
app.message
listener types in this pull request. Most developers will be happy with the change because, with the changes in this PR, developers no longer need to have the if/else blocks. One downside of this approach is that other subtype patterns, such asmessage_changed
, won't be delivered toapp.message
listeners anymore. This behavioral modification can be a breaking change to some of the existing apps. For those apps, I've addedapp.allMessageSubtypes
listener, which is fully compatible with the currentapp.message
listener's behavior.I would love to hear what other maintainers and community members think abou this proposal! If it looks good to many, we may merge the change in a future major or minor release. In theory, we should have this change in the next major but it may take long. Is it worth considering to have it in a minor release? Or, should we plan releasing next major soon?
Requirements (place an
x
in each[ ]
)