Skip to content
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

Merged
merged 4 commits into from
Aug 30, 2024

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Aug 29, 2024

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.

@filmaj filmaj added semver:minor enhancement M-T: A feature request for new functionality area:typescript issues that specifically impact using the package from typescript projects pkg:types applies to `@slack/types` labels Aug 29, 2024
@filmaj filmaj added this to the [email protected] milestone Aug 29, 2024
@filmaj filmaj requested a review from a team August 29, 2024 22:23
@filmaj filmaj self-assigned this Aug 29, 2024
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.26%. Comparing base (be8cb4e) to head (39156ce).
Report is 1 commits behind head on main.

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           
Flag Coverage Δ
cli-hooks 94.94% <ø> (ø)
cli-test 96.93% <ø> (ø)
oauth 76.53% <ø> (ø)
socket-mode 59.59% <ø> (ø)
web-api 96.57% <ø> (ø)
webhook 95.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@misscoded misscoded left a 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! :shipit:

packages/types/src/events/reaction.ts Show resolved Hide resolved
packages/types/src/events/shared-channel.ts Show resolved Hide resolved
packages/types/src/events/steps-from-apps.ts Show resolved Hide resolved
packages/types/src/events/message.ts Outdated Show resolved Hide resolved
packages/types/src/common/bot-profile.ts Show resolved Hide resolved
deleted: boolean;
}

interface File {
Copy link
Contributor

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?

Copy link
Contributor Author

@filmaj filmaj Aug 29, 2024

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.

Copy link
Member

@srajiang srajiang left a 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.

@filmaj filmaj merged commit d857511 into main Aug 30, 2024
30 checks passed
@filmaj filmaj deleted the types-event-payloads branch August 30, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typescript issues that specifically impact using the package from typescript projects enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

types: add event payloads
3 participants