Add missing message events & more type tests #832

seratch · 2021-03-15T05:44:12Z

Summary

This pull request adds a few subtyped message events to top-level exports (currently, only BotMessageEvent is available in from @slack/bolt`). Also, as the very first step for #826 improvements, I've added a few more tests with TODO comments.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2021-03-15T05:45:43Z

Codecov Report

Merging #832 (7c6c294) into main (74368d2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #832   +/-   ##
=======================================
  Coverage   65.89%   65.89%           
=======================================
  Files          13       13           
  Lines        1173     1173           
  Branches      343      343           
=======================================
  Hits          773      773           
  Misses        334      334           
  Partials       66       66

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74368d2...868a2eb. Read the comment docs.

seratch

left comments for reviewers and anyone who are interested

seratch · 2021-03-15T05:44:29Z

src/types/events/index.ts

@@ -1,5 +1,14 @@
 export * from './base-events';
-export { BotMessageEvent, GenericMessageEvent } from './message-events';
+export {


This is the only change to the main code side.

seratch · 2021-03-15T05:49:56Z

types-tests/message.test-d.ts


 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 }) => {});


We haven't agreed to implement this yet but this could be beneficial for some use cases. There are a few more things to consider:

Should we enable developers to pass multiple subtypes here?

If yes, the message object type in the scenario is not fully deterministic (it can be a bit more specific than MessageEvent but it's still a union type).

If no, for handling multiple types of subtyped message events, the right way is to have different event listeners for each subtype (some of internal code can be reused).

Is pattern a good key name for the pattern here?

app.message<"message_replied"> vs app.message(constraints or should we support both?

app.action and app.options are not consistent at this point (refer to my comments below)

I like the idea of having a constraints object here. subtype in the constraints object makes sense. pattern seems like a reasonable key name.

If we wanted to support multiple subtypes in one listener, what would that look like? An array for subtype: ['message_replied']. Having an array in a constraint object would be new, but if using one listener for multiple subtypes is a usecase we think is common and should handle, it makes sense. Maybe @shaydewael can chime in on this.

My gut feeling is app.message(constraints is more important. Does app.options not support type in constraints?

We have another pull request making changes to the app.message listener signature: #761

one more suggestion for this PR: can we add some type tests within type-tests/message.test-d.ts that illustrate (and validate) the expected usage?

Although Ankur asked the contributor to add more tests, we can handle the tests for the developer. Should we merge this PR before starting to work on the change we're discussing here?

@stevengill

My gut feeling is app.message(constraints is more important. Does app.options not support type in constraints?

As I mentioned the added tests for app.options in this PR, it just does not work. I think we should make the developer experience consistent across the listeners but currently there are some gaps. Refer to the TODO/FIXME comments in this PR for details.

seratch · 2021-03-15T05:51:32Z

types-tests/options.test-d.ts

+}));
+
+// block_suggestion
+expectType<void>(app.options<'block_suggestion'>({ action_id: 'a' }, async ({ options }) => {


Only app.options has the string type parameter like this. app.view and app.action do not support the same way while they can determine payload type in accordance with type in a constraint object.

stevengill · 2021-03-15T18:18:27Z

Thanks for adding all of these great type tests.

jestrada · 2021-12-22T03:31:29Z

src/types/events/index.ts

+  MessageDeletedEvent,
+  ThreadBroadcastMessageEvent,
+  MessageChangedEvent,
+  EKMAccessDeniedMessageEvent,


Is there any chance we can export all the message events? Why is the list limited to a subset of events?

@jestrada Do you have anything missing in your mind?

Hey @seratch, thanks for the reply! Ya, all of the Channel*MessageEvent types are missing which makes it impossible to refer to import them. Is it possible to export all message event types.

Reference: https://github.com/slackapi/bolt-js/blob/main/src/types/events/message-events.ts#L3

Ah I got the point. This is an easy fix. Thanks for pointing this out.

submitted a PR to fix this: #1254

Thanks @seratch!

Add missing message events & more type tests

868a2eb

seratch added enhancement M-T: A feature request for new functionality TypeScript-specific labels Mar 15, 2021

seratch requested review from mwbrooks and stevengill March 15, 2021 05:44

seratch self-assigned this Mar 15, 2021

seratch commented Mar 15, 2021

View reviewed changes

stevengill approved these changes Mar 15, 2021

View reviewed changes

mwbrooks approved these changes Mar 15, 2021

View reviewed changes

seratch merged commit 3ed08fc into slackapi:main Mar 17, 2021

seratch deleted the missing-message-events-more-tests branch March 17, 2021 08:55

seratch mentioned this pull request Mar 22, 2021

Developer experience building Slack apps with Bolt using TypeScript #826

Closed

10 tasks

seratch added this to the 3.4.0 milestone Mar 23, 2021

stevengill mentioned this pull request Mar 29, 2021

Typescript missing props on message #861

Closed

6 tasks

jestrada reviewed Dec 22, 2021

View reviewed changes

seratch mentioned this pull request Dec 22, 2021

Export more message event types #1254

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing message events & more type tests #832

Add missing message events & more type tests #832

seratch commented Mar 15, 2021

codecov bot commented Mar 15, 2021 •

edited

Loading

seratch left a comment

seratch Mar 15, 2021

seratch Mar 15, 2021

stevengill Mar 15, 2021

seratch Mar 15, 2021

seratch Mar 15, 2021

stevengill commented Mar 15, 2021

jestrada Dec 22, 2021

seratch Dec 22, 2021

jestrada Dec 22, 2021

seratch Dec 22, 2021

seratch Dec 22, 2021

jestrada Dec 22, 2021

Add missing message events & more type tests #832

Add missing message events & more type tests #832

Conversation

seratch commented Mar 15, 2021

Summary

Requirements (place an x in each [ ])

codecov bot commented Mar 15, 2021 • edited Loading

Codecov Report

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengill commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Requirements (place an `x` in each `[ ]`)

codecov bot commented Mar 15, 2021 •

edited

Loading