-
Notifications
You must be signed in to change notification settings - Fork 662
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
feat: adding event payloads from bolt-js as a starting point #1907
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1907 +/- ##
=======================================
Coverage 90.26% 90.26%
=======================================
Files 34 34
Lines 7666 7666
Branches 381 381
=======================================
Hits 6920 6920
Misses 734 734
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
The division of payloads makes sense to me, though the message
one gave me the most pause since there seems to be some overlap with other things (and a rogue message
event being housed elsewhere, as well).
Left some comments about non-exported items, and what I think are duplicates. Other than that, LGTM!
deleted: boolean; | ||
} | ||
|
||
interface File { |
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.
Should this be in file.ts
instead?
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.
No, because file.ts
holds the file-related event shapes, while here within message.ts
we house message event related shapes, which look like contain file-related information (thus the existence of this interface). For whatever reason, the file-related events don't send file info 😆 that might be a bug / gap today, but I don't want to be fixing or changing things, as I want to maintain whatever existing shape compatibility this code has so that no breaking changes are introduced in bolt (which will immediately consume all this as soon as this PR is published - WIP PR here: slackapi/bolt-js#2223)
I think once I get to the refactoring task (#1904), I will take a look at the entire types package holistically and see where else we need a File
interface and make sure it's reused appropriately.
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.
Naming schema very sensible, I left a suggestion around the channel-membership file.
fixes #1395
Summary
This PR doesn't do much other than copy, and slightly split up into separate files, the bolt-js types present here: https://github.com/slackapi/bolt-js/tree/main/src/types/events
(I have a WIP PR for bolt-js that passes all tests w/ this
types
PR here: slackapi/bolt-js#2223)The Big Idea
Let's move event payloads into this
types
package, since it revisions stuff like "shapes of things." Why? While to date these types have existed in bolt-js, they could be reused in other packages: socket-mode (see #1768) and deno-slack-sdk, for one (yes, deno packages, too, can consume@slack/types
!).This should be a backwards compatible addition, and nothing in bolt should break as a result of this move - step one is just shifting code around. Step two, though, is much more interesting: what sorts of breaking changes can we introduce in a new types v3 package that improves the lives of our developers? Very interesting question that is tracked in #1904.