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

Fix #1240 by enabling to pass integer as post_at in chat.scheduleMessage API calls #1242

Merged
merged 1 commit into from
May 20, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented May 20, 2021

Summary

This pull request fixes #1240 by correcting the argument type for post_at in ChatScheduleMessageArguments. The argument should accept either number (specifically integer value) or string value for it. Although both number and string actually works, the API document https://api.slack.com/methods/chat.scheduleMessage recommends number over string.

Requirements (place an x in each [ ])

@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch area:typescript issues that specifically impact using the package from typescript projects pkg:web-api applies to `@slack/web-api` labels May 20, 2021
@seratch seratch added this to the web-api@6.2.4 milestone May 20, 2021
@seratch seratch self-assigned this May 20, 2021
@@ -1225,7 +1225,7 @@ export interface ChatPostMessageArguments extends WebAPICallOptions, TokenOverri
export interface ChatScheduleMessageArguments extends WebAPICallOptions, TokenOverridable {
channel: string;
text?: string;
post_at: string;
post_at: string | number;
Copy link
Contributor

Choose a reason for hiding this comment

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

One thought: by not limiting it to only number, there's a greater possibility that folks will continue to interpret string as possibly being a timestamp, as the dev in this case did. By switching it to number, we reduce the chance of that happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

@misscoded As I mentioned in this PR's description and my response to the developer, using string here is totally fine. It works. The issue the developer had was that they used non-integer value. For this reason, we still can have string as an acceptable type for it. If we remove string from here, it's a breaking change.

Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

LGTM

@seratch
Copy link
Member Author

seratch commented May 20, 2021

Thanks for your reviews!

@seratch seratch merged commit 3284f56 into slackapi:main May 20, 2021
@seratch seratch deleted the issue-1240-chat-scheduleMessage branch May 20, 2021 23:51
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 bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:web-api applies to `@slack/web-api` semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

post_at as option in schedule message has the wrong type
3 participants