fix(web-api): update thread_ts arg to be required for chat.startStream method
#2382
Conversation
mwbrooks
left a comment
There was a problem hiding this comment.
Notes for the kind reviewers! 🙇🏻
| Partial<ThreadTS>, | ||
| Partial<MarkdownText>, | ||
| Unfurls { | ||
| export interface ChatStartStreamArguments extends TokenOverridable, Channel, Partial<MarkdownText>, ThreadTS, Unfurls { |
There was a problem hiding this comment.
note: npm test linter requires this to be a single-line. I ordered it alphabetically.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ai-apps #2382 +/- ##
==========================================
Coverage ? 92.78%
==========================================
Files ? 39
Lines ? 10711
Branches ? 692
==========================================
Hits ? 9938
Misses ? 761
Partials ? 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
@mwbrooks Thanks so much for a fast update on this once more! ⭐
I marked this as a patch change since I believe aligning to the API methods can be considered a bugfix with invalid arguments erroring later anyways...
We should add this to a prerelease soon IMHO! 🚢 💨
| // chat.startStream | ||
| // -- sad path | ||
| expectError(web.chat.startStream()); // lacking argument | ||
| expectError(web.chat.startStream({})); // empty argument |
There was a problem hiding this comment.
| expectError(web.chat.startStream({})); // empty argument | |
| expectError(web.chat.startStream({ | |
| channel: 'C1234', // missing thread_ts | |
| })); |
🧪 suggestion(non-blocking): Asserting that the previous behavior errors might be nice to have, but no blocker for this PR!
|
Thanks for the quick review @zimeg and solid suggestion on covering the edge-case tests. 🙇🏻 I like that you've added the |
Summary
This PR updates the
chat.startStreamarguments to now requirethread_ts. We are changing this because the Slack API has been updated to now require thethread_tswhen starting a stream. This means all DMs and in-channel conversations with your bot will now have responses replied in-thread.After merging, we'll need to update our sample apps to always provide
thread_ts.Requirements (place an
xin each[ ])