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 #894 Unable to build options request objects in TypeScript #900

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Apr 28, 2021

Summary

This pull request resolves #894 by correcting the types for external data source request payloads. @trevor-gullstad flagged this issue by sending pull request #893 (thank you very much!). Refer to the issue and my comments in this pull request for more details.

Requirements (place an x in each [ ])

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented TypeScript-specific labels Apr 28, 2021
@seratch seratch added this to the 3.4.0 milestone Apr 28, 2021
@seratch seratch self-assigned this Apr 28, 2021
@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #900 (26b89ce) into main (a5ac8fd) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #900   +/-   ##
=======================================
  Coverage   66.19%   66.19%           
=======================================
  Files          13       13           
  Lines        1204     1204           
  Branches      355      355           
=======================================
  Hits          797      797           
  Misses        338      338           
  Partials       69       69           
Impacted Files Coverage Δ
src/middleware/builtin.ts 84.09% <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 a5ac8fd...26b89ce. Read the comment docs.

Copy link
Member Author

@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.

a few comments for reviewers

export interface OptionsRequest<Source extends OptionsSource = OptionsSource> extends StringIndexed {
export type OptionsSource = 'interactive_message' | 'dialog_suggestion' | 'block_suggestion';

export type SlackOptions = BlockSuggestion | InteractiveMessageSuggestion | DialogSuggestion;
Copy link
Member Author

Choose a reason for hiding this comment

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

This may sound a bit unnatural but this is consistent with other union types (e.g., SlackAction, SlackEvent, SlackShortcut, SlackViewAction)

type: Source;
}

type OptionsPayloadFromType<T extends string> = KnownOptionsPayloadFromType<T> extends never
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same approach with Events API payload handling.

/**
* external data source in blocks
*/
export interface BlockSuggestion extends StringIndexed {
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to remove StringIndexed but it's not backward compatible.

team: {
id: string;
domain: string;
enterprise_id?: string; // undocumented
Copy link
Member Author

Choose a reason for hiding this comment

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

"undocumented " here could be outdated comments

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 seratch merged commit c88aeb7 into slackapi:main Apr 29, 2021
@seratch seratch deleted the issue-894-options-types branch April 29, 2021 21:53
@seratch seratch mentioned this pull request Apr 29, 2021
2 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 TypeScript-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to build options request objects in TypeScript
2 participants