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

Update message types #782

Merged
merged 6 commits into from
Feb 10, 2021

Conversation

shaydewael
Copy link
Contributor

Summary

Addresses #774 and updates other fields that were missing

Requirements (place an x in each [ ])

@shaydewael
Copy link
Contributor Author

For context, I found the file type here: https://api.slack.com/types/file

@@ -13,16 +13,23 @@ export type MessageEvent =
export interface GenericMessageEvent {
type: 'message';
subtype: undefined;
event_ts: string;
team: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this field does not always exist, we may want to make it optional.

text: string;
blocks?: (KnownBlock | Block)[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, attachments is missing.

@seratch
Copy link
Member

seratch commented Feb 9, 2021

Can you update the spec file to be compatible with the new type definition?

TSError: ⨯ Unable to compile TypeScript:
Error: src/middleware/builtin.spec.ts(752,7): error TS2322: Type '{ type: "message"; subtype: "bot_message"; channel: string; user: string; ts: string; text: string; bot_id: string; channel_type: "channel"; }' is not assignable to type 'MessageEvent'.
  Property 'event_ts' is missing in type '{ type: "message"; subtype: "bot_message"; channel: string; user: string; ts: string; text: string; bot_id: string; channel_type: "channel"; }' but required in type 'BotMessageEvent'.

Apart from this build failure, this PR looks great to me 👍

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge this once the build failure is fixed.

@seratch
Copy link
Member

seratch commented Feb 9, 2021

This branch still have a lint error. You need to apply code formatter to the modified spec files. Can you run npx prettier -w src/ && npm test and push the change?

@stevengill stevengill added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:minor and removed untriaged labels Feb 10, 2021
@stevengill stevengill merged commit 5e2db65 into slackapi:main Feb 10, 2021
@seratch seratch mentioned this pull request Mar 5, 2021
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants