Improve types for message events and all subtypes by aoberoi · Pull Request #709 · slackapi/bolt-js

aoberoi · 2020-12-14T07:37:38Z

Summary

Previously, the SlackEvent union included a type called MessageEvent which represents all Events API payloads with type:'message'. However, the union didn't include the message event subtypes (such as subtype='bot_message').

With this change, MessageEvent is itself a union of all the subtypes. That union is still part of the overall SlackEvent union. This means that both the app.event() and app.message() methods are now aware of message subtypes.

Fixes #311

Another related change is also in this PR: the index type has been removed from all events in the SlackEvent union. This means you can no longer safely read any property name from any event and treat the value as an any.

NOTE: This will cause new type checking errors where there wasn't one before in some apps. Those errors will be legitimate (unsafe accesses to event properties), but these apps may have been working fine until now and builds could be broken. This is actually a feature of using TypeScript, this isn't considered a breaking change.

If you're reading this because your build broke and you're looking for a quick fix, it may be helpful to know two things:

If you know that the property you're trying to access is available on the event, here is how you should approach it. First, maybe the types in @slack/bolt are out of date or wrong - consider creating an issue in the repo. Second, you can use a cast to say "I know better than Bolt's types" and immediately unblock yourself.
The type that was previously called MessageEvent is now called GenericMessageEvent. This new type is meant to be used to represent Events API messages of type 'message', but which do not have a 'subtype'. You can likely get your build working again by intentionally casting the message argument inside app.message() listeners to the GenericMessageEvent type. But before you do this, consider taking any errors seriously as you may actually have an unsafe access to a property. You can use a conditional check to narrow the type of message and safely use properties like below:

app.message(async ({ message }) => {
  // message is type MessageEvent here
  if (message.subtype === 'message_changed') {
    // message is type MessageChangedEvent here
    message.message // this is now a safe access and TypeScript won't error
  }
  // accessing message.message outside of the previous conditional is unsafe and TypeScript will error
});

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.

Moved all the subtypes into a new file to keep them more organized. Wrote a test file that should theoretically pass when this issue is fixed. Does not yet pass TS compiler, more changes to come.

Once we removed StringIndexed from SlackEvent types, many property lookups became typechecking errors. I borrowed this refactor mostly from a WIP PR to improve our style/linting in a proactive effort to redue conflicts when that PR lands. The only changes I needed to add were the Org Apps changes that had landed on main. slackapi#669 In a few cases, the event types had other issues that became apparent only after removing the aggressive casting we had before. Those are fixed. It's now evident that there's more type related problems with respect to the Authorize, its related types, especially the OrgApps variants. I'll try to fix these next. It's also not clear if buildSource needs the orgInstall argument. Will seek advice in code review for that.

By making these two types generic, we can resolve the typing issue in buildSource and a couple other places. This code still doesn't compile. May need to update singleTeamAuthorization. Getting close.

In both App.ts and builtin.ts, issues were resolve simply by refactoring to an equivalent form that is more type-safe. In App.ts specifically, processEvent() was refactored to reduce some code repetition as well. In helpers.ts this actually fixes bugs. The getTypeAndConversation() would have failed conversation lookup for the following event types: pin_added, pin_removed, call_rejected, channel_created, channel_rename, group_rename, im_created.

codecov · 2020-12-14T07:39:07Z

Codecov Report

Merging #709 (aaf9fbd) into main (8c7434a) will increase coverage by 0.53%.
The diff coverage is 68.57%.

@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
+ Coverage   82.32%   82.85%   +0.53%     
==========================================
  Files           8        8              
  Lines         758      770      +12     
  Branches      250      230      -20     
==========================================
+ Hits          624      638      +14     
- Misses         78       87       +9     
+ Partials       56       45      -11

Impacted Files	Coverage Δ
src/helpers.ts	`80.95% <58.33%> (-9.96%)`	⬇️
src/App.ts	`84.51% <68.88%> (+2.43%)`	⬆️
src/middleware/builtin.ts	`82.78% <100.00%> (ø)`

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 8c7434a...aaf9fbd. Read the comment docs.

aoberoi · 2020-12-14T07:41:34Z

src/App.ts

-      conversationId: channelId,
-      isEnterpriseInstall: false,
-    };
+


This refactor is mostly borrowed from #669, so that when it eventually lands, there's less chance of a merge conflict.

I found all the Org Apps related changes on main and layered them into this part of code from the linting PR.

Prettier undid some of my style related changes, but these will likely be automatically fixed again once the linting PR lands.

I'm a big fan of this refactor. Much easier to read and follow

aoberoi · 2020-12-14T07:44:28Z

src/types/events/base-events.ts


 /* ------- TODO: Generate these interfaces ------- */

-// TODO: why are these all StringIndexed? who does that really help when going more than one level deep means you have


Once I removed StringIndexed as a base from all the event interfaces, many unsafe property accesses in our own codebase became type-checking errors. Most of refactoring in the rest of this PR is simply an attempt to address those errors.

aoberoi · 2020-12-14T07:47:45Z

src/App.ts

-  conversationId?: string;
-  isEnterpriseInstall?: boolean;
+  isEnterpriseInstall: IsEnterpriseInstall;
 }


This is likely the most meaningful refactor. I've done something similar here to what I did in node-slack-sdk - consolidated the types that were very similar but just varied on IsEnterpriseInstall by making the types generic.

This allowed me to safely reduce code repetition within App.processEvent() and singleTeamAuthorization() (now singleAuthorization()).

aoberoi · 2020-12-14T07:51:15Z

src/App.ts

 }

-function singleTeamAuthorization(
+function singleAuthorization(


The rename is because this function addresses authorization on a single team OR a single enterprise. I considered having two separate functions for those purposes, but realized the body would look mostly identical except for the value of isEnterpriseInstall in the return value. That could easily be consolidated because isEnterpriseInstall is actually passed to the Authorize function as a property of source anyway.

aoberoi · 2020-12-14T07:54:17Z

src/helpers.ts

+    const { event } = body as SlackEventMiddlewareArgs<string>['body'];
+
+    // Find conversationId
+    const conversationId: string | undefined = (() => {


This code actually got a bit longer because it fixes some actual bugs (not just a straight refactor). Previously, events of types pin_added, pin_removed, call_rejected, channel_created, channel_rename, group_rename, and im_created would not have a conversationId in the return value because the conditional checks were incomplete.

aoberoi · 2020-12-14T07:55:38Z

src/App.ts

+  private authorize!: Authorize<false>;

  /** Org Authorize */
-  private orgAuthorize!: Authorize;


This was technically wrong previously. The Authorize type describes a function which non-optionally took a teamId.

aoberoi · 2020-12-14T07:57:47Z

src/App.ts

+  };
+}
+
+function isBodyWithTypeEnterpriseInstall(body: AnyMiddlewareArgs['body'], type: IncomingEventType): boolean {


New helper function so the calling code is more readable.

seratch · 2020-12-15T03:02:04Z

Regarding the removal of StringIndexed and the better (magical) way to determine sub types in message events look great to me!

It seems this pull request consists of the following changes. And, if I understand correctly, 3 & 4 changes do not rely on 1 & 2.

1 refactoring authorize in accordance with org-wide app support
2 bugfix on conversation id extraction in getTypeAndConversation
3 StringIndexed removal for all events
4 Improvements on message event types

I haven't checked the details for authorize changes and a bug fix in getTypeAndConversation yet. I'm fine to have 3 & 4 in this PR but I think we can have separate PRs for 1 & 2. I personally think 1 & 2 are already great but having only 3 & 4 in this PR makes reviews much easier. We still have other tasks for the next major version. So, I think trying to minimize each pull request and quickly merge should help us smoothly deliver more changes.

seratch

my approval for the event payload related parts

seratch · 2020-12-15T05:10:01Z

src/types/events/message-events.ts

+  | MessageRepliedEvent
+  | ThreadBroadcastMessageEvent;
+
+export interface GenericMessageEvent {


The type definition seems to be missing some fields such as team and channel_type. You can check the Java SDK's type definition to learn them. https://github.com/slackapi/java-slack-sdk/tree/v1.4.0/slack-api-model/src/main/java/com/slack/api/model/event

Great catch! Should I create another issue for this so it can be addressed in a separate PR? This PR was meant to solve the structural issue with the union types, but the issue you've pointed out is independent of that.

Let's create a new issue for it!

stevengill · 2020-12-16T23:21:10Z

src/App.ts

-      conversationId: channelId,
-      isEnterpriseInstall: false,
-    };
+


I'm a big fan of this refactor. Much easier to read and follow

src/App.ts

stevengill · 2020-12-16T23:34:30Z

src/App.ts

+
+      // When the app is installed using org-wide deployment, team property will be null
+      if (bodyAsActionOrOptionsOrViewActionOrShortcut.team !== null) {
+        return bodyAsActionOrOptionsOrViewActionOrShortcut.team.enterprise_id;


I wonder if this check should be moved below line 850 check. Why would body.team.enterprise_id take priority over body.enterprise.id?

stevengill · 2020-12-16T23:48:45Z

src/App.ts


-  return async () => {
-    return { botToken: authorization.botToken, ...(await identifiers) };
+  return async ({ isEnterpriseInstall }) => {


probably just a brain fart, but where is this isEnterpriseInstall coming from here?

Yeah higher order functions can be mind-bending for me too. singleAuthorization() returns a function, which as the type Authorize. So this expression is of type Authorize. Which means the argument is of type AuthorizeSourceData. So this isEnterpriseInstall is the property of AuthorizeSourceData, defined on line 94.

As for the actual value, its from the return value of buildSource().

Co-authored-by: Steve Gill <stevengill97@gmail.com>

aoberoi · 2020-12-17T13:35:08Z

I'm fine to have 3 & 4 in this PR but I think we can have separate PRs for 1 & 2.

@seratch I initially started this PR to only work on 3 & 4, but as soon as those were fixed (specifically removing StringIndexed) the build was failing because of the unsafe accesses. Then I started working on 1 & 2 to fix those unsafe accesses.

I agree that keeping PRs small and focused makes them easier to review, and easier to land when there's several changes worked on by different people. In order to do that here, I think I have to first land the changes related to 1 & 2 and then land the changes related to 3 & 4 (since the changes in 3 & 4 will not build otherwise). This PR would represent the latter (that's already the portion you reviewed). I'll give that a try.

stevengill · 2020-12-17T18:51:33Z

Instead of spending time breaking this into two, I suggest we move forward with merging this PR! It is looking pretty solid to me!

src/App.ts

Co-authored-by: Steve Gill <stevengill97@gmail.com>

aoberoi added 6 commits December 11, 2020 00:52

Add message event subtypes to SlackEvent union

96c961e

Moved all the subtypes into a new file to keep them more organized. Wrote a test file that should theoretically pass when this issue is fixed. Does not yet pass TS compiler, more changes to come.

Generalize Authorize and AuthorizeSourceData types

d8d344e

By making these two types generic, we can resolve the typing issue in buildSource and a couple other places. This code still doesn't compile. May need to update singleTeamAuthorization. Getting close.

prettier

c1da2d7

Fix up test for changes

aa6234c

gitwave bot added the untriaged label Dec 14, 2020

aoberoi commented Dec 14, 2020

View reviewed changes

aoberoi added 2 commits December 13, 2020 23:59

expose GenericMessageEvent to ease fixing broken builds

39b8fd2

Merge branch 'main' into fix-loosely-indexed-event-types

22731fb

aoberoi requested review from seratch and stevengill December 14, 2020 08:03

seratch approved these changes Dec 15, 2020

View reviewed changes

seratch reviewed Dec 15, 2020

View reviewed changes

stevengill added enhancement M-T: A feature request for new functionality semver:minor and removed untriaged labels Dec 16, 2020

stevengill reviewed Dec 16, 2020

View reviewed changes

Update src/App.ts

8266e68

Co-authored-by: Steve Gill <stevengill97@gmail.com>

stevengill approved these changes Dec 17, 2020

View reviewed changes

stevengill reviewed Dec 17, 2020

View reviewed changes

src/App.ts Outdated Show resolved Hide resolved

aoberoi and others added 3 commits January 4, 2021 12:29

Merge branch 'main' into fix-loosely-indexed-event-types

b7bd774

Update src/App.ts

86340c9

Co-authored-by: Steve Gill <stevengill97@gmail.com>

Merge branch 'main' into fix-loosely-indexed-event-types

aaf9fbd

aoberoi merged commit 30af88b into slackapi:main Jan 4, 2021

aoberoi deleted the fix-loosely-indexed-event-types branch January 4, 2021 20:39

BenAlderfer mentioned this pull request Jan 7, 2021

What is the proper way to get the thread ts now? #735

Closed

10 tasks

misscoded mentioned this pull request Jan 9, 2021

Add thread_ts to AppMentionEvent interface #739

Merged

2 tasks

seratch mentioned this pull request Jan 29, 2021

MessageEvent type missing user #774

Closed

10 tasks

seratch mentioned this pull request Jun 3, 2021

Infer Slack Bolt TypeScript types #955

Closed


		/* ------- TODO: Generate these interfaces ------- */

		// TODO: why are these all StringIndexed? who does that really help when going more than one level deep means you have

Conversation

aoberoi commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Requirements (place an x in each [ ])

Uh oh!

codecov bot commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aoberoi Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aoberoi Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aoberoi Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aoberoi commented Dec 17, 2020

Uh oh!

stevengill commented Dec 17, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aoberoi commented Dec 14, 2020 •

edited

Loading

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

codecov bot commented Dec 14, 2020 •

edited

Loading

aoberoi Dec 14, 2020 •

edited

Loading

aoberoi Dec 14, 2020 •

edited

Loading

seratch commented Dec 15, 2020 •

edited

Loading

aoberoi Dec 17, 2020 •

edited

Loading