-
Notifications
You must be signed in to change notification settings - Fork 399
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
Support SlackFunction and Function Localized Interactivity handling #1567
Conversation
265a89f
to
c9981db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some notes for reviewers. Thanks in advance for your 👀 !
* Register subcription middleware | ||
* Register WorkflowStep middleware | ||
* | ||
* Not to be confused with next-gen platform Workflows + Functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds some documentation to indicate WorkflowStep is an unrelated legacy feature.
@@ -46,83 +113,412 @@ export class SlackFunction { | |||
private callbackId: string; | |||
|
|||
/** | |||
* @description fn to to process corresponding | |||
* @description handler to to process corresponding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deno Slack SDK is using the terminology "handlers" to I am being consistent with them.
if (manifestJS) { | ||
manifest = merge(manifest, manifestJS, { arrayMerge: unionMerge }); | ||
} | ||
let manifest = getManifestData(cwd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the body of this logic out into a more generalized getManifestData()
which can also be called from other parts of App
or from SlackFunction
.
c9981db
to
73e0f1e
Compare
d40ec6e
to
a32dd14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested out the app, happy path works great!
Sad path (i.e. introduce an exception in userland code) blows up the process. Left a comment in / near / around the code area. Not sure what we expect to happen in that situation.
Overall excellent work!
P.S. thanks for adding the JSDocs while you were at it 🙇
src/SlackFunction.ts
Outdated
this.logger.debug('🚀 Executing my main handler:', this.handler); | ||
return await this.runHandler(args); | ||
} catch (error) { | ||
this.logger.error('⚠️ Something went wrong executing:', this.handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to check what this log looks like but I couldn't trip it.
I edited the function body to throw an error, then this is the output I see:
[DEBUG] socket-mode:SocketModeClient:0 Received a message on the WebSocket: {"envelope_id": ... very long payload here truncated}
[DEBUG] socket-mode:SocketModeClient:0 Calling ack() - type: events_api, envelope_id: 523882e6-c036-43ba-8e7a-e5c15c7f7007, data: undefined
[DEBUG] socket-mode:SocketModeClient:0 send() method was called in state: connected, state hierarchy: connected,ready
[DEBUG] socket-mode:SocketModeClient:0 Sending a WebSocket message: {"envelope_id":"523882e6-c036-43ba-8e7a-e5c15c7f7007","payload":{}}
[DEBUG] bolt-app initialized
[DEBUG] bolt-app No conversation ID for incoming event
[DEBUG] SlackFunction: [review_approval] 🚀 Executing my main handler: [AsyncFunction: notifyApprover]
/Users/fmaj/src/slapp-BoltJSHermes/listeners/functions/request-approval.js:12
throw new Error('boomsies');
^
Error: boomsies
at SlackFunction.notifyApprover [as handler] (/Users/fmaj/src/slapp-BoltJSHermes/listeners/functions/request-approval.js:12:9)
at SlackFunction.runHandler (/Users/fmaj/src/bolt-js/dist/SlackFunction.js:60:18)
at Array.<anonymous> (/Users/fmaj/src/bolt-js/dist/SlackFunction.js:184:39)
at invokeMiddleware (/Users/fmaj/src/bolt-js/dist/middleware/process.js:12:53)
at next (/Users/fmaj/src/bolt-js/dist/middleware/process.js:13:29)
at Array.<anonymous> (/Users/fmaj/src/bolt-js/dist/conversation-store.js:67:15)
at invokeMiddleware (/Users/fmaj/src/bolt-js/dist/middleware/process.js:12:53)
at Object.next (/Users/fmaj/src/bolt-js/dist/middleware/process.js:13:29)
at Array.<anonymous> (/Users/fmaj/src/bolt-js/dist/middleware/builtin.js:273:20)
at invokeMiddleware (/Users/fmaj/src/bolt-js/dist/middleware/process.js:12:53)
bolt-app local run exited with code 1
Note also that this kills the entire process. Pretty weird as I would assume this try/catch block would catch it. Guess not. Is this expected? I would have expected to see "Something went wrong", and possibly also for the process not to die. Whatever the expectation is, let's make sure to add a test for this behaviour if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so the return await
ing in the line you highlighted looks right, but I missed an await
in runHandler, so that was immediately fulfilling with undefined, and then getting returned out all the way up, bypassing my well-intentioned catch block. I've simplified code little bit and I think it's much clearer now. Added a test for the case as well.
Codecov Report
@@ Coverage Diff @@
## future #1567 +/- ##
=========================================
Coverage ? 80.61%
=========================================
Files ? 20
Lines ? 1764
Branches ? 502
=========================================
Hits ? 1422
Misses ? 221
Partials ? 121 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -93,7 +93,7 @@ | |||
"source-map-support": "^0.5.12", | |||
"ts-node": "^8.1.0", | |||
"tsd": "^0.22.0", | |||
"typescript": "^4.1.0" | |||
"typescript": "4.7.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caret let 4.8.2 of typescript into the mix. This version has breaking changes, which were causing ts compiler complaints on npm install of @slack/deno-slack-sdk
. Pinning to the latest compatible version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @filmaj for the review! 🙇 I think I've resolved all of the inline comments!
src/SlackFunction.ts
Outdated
this.logger.debug('🚀 Executing my main handler:', this.handler); | ||
return await this.runHandler(args); | ||
} catch (error) { | ||
this.logger.error('⚠️ Something went wrong executing:', this.handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so the return await
ing in the line you highlighted looks right, but I missed an await
in runHandler, so that was immediately fulfilling with undefined, and then getting returned out all the way up, bypassing my well-intentioned catch block. I've simplified code little bit and I think it's much clearer now. Added a test for the case as well.
@@ -1,4 +1,8 @@ | |||
/** | |||
* Dialogs were a way to trigger interactions with users in messaging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a note about this feature as being legacy (though not deprecated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeeeuhhhh epic work 🥇
@@ -55,7 +56,7 @@ export interface ViewSubmitAction { | |||
view: ViewOutput; | |||
api_app_id: string; | |||
token: string; | |||
trigger_id: string; // undocumented | |||
trigger_id?: string; // undocumented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trigger_id and token changes here as well as the similar change in BlockActions type is to reflect the fact that now in certain cases (aka when the payload is sent with FunctionContext
, the trigger_id will NOT be included).
actionIdOrConstraints; | ||
|
||
// declare our valid constraints keys | ||
const validConstraintsKeys: ActionConstraintsKeys = ['action_id', 'block_id', 'callback_id', 'type']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constraints keys types are here to help us make sure we're never declaring validConstraintsKeys that are not corresponding to the underlying ActionConstraints or ViewsConstraints types.
e124960
to
9d6d22f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍 Thanks a lot for working on this
812f851
to
de0b2c2
Compare
…1567) * Support SlackFunction and Function Localized Interactivity handling
…1567) * Support SlackFunction and Function Localized Interactivity handling
…1567) * Support SlackFunction and Function Localized Interactivity handling
…1567) * Support SlackFunction and Function Localized Interactivity handling
…1567) * Support SlackFunction and Function Localized Interactivity handling
…1567) * Support SlackFunction and Function Localized Interactivity handling
…1567) * Support SlackFunction and Function Localized Interactivity handling
Todo:
Some failing CI/CD, but it looks like it's related to building the @slack/deno-slack-sdk types. And I don't see this issue when I npm install and build locally, so need to investigateIssue was due to breaking change that snuck it's way into a minor(?) version oftypescript
released on Friday. Pinning to v.4.7.4 resolved the issue for now. Long term we might want to revisit whether or not to keep the version of TS pinned.Summary
Adds support for Function Localized Interactivity.
Adds a
SlackFunction
classExample:
Example:
complete() - ing one's function
.Completing your function
Call the supplied utility
complete()
in any handler with either outputs or an error or nothing when your function is done.This tells Slack whether to proceed with any next steps in any workflow this function might be included in. Your outputs should match what is defined in your manifest.
Example:
Dev Experience
Try it out yourself:
hermes create -t slack-samples/take-your-time -b initial-submission
Use this branch of
bolt-js
on this branchnpm link
, then in the sample projectnpm link @slack/bolt
Here's an example of a best-practice function file.
Note we can add out Slack Function interactivity handlers with constraints / regex
SlackFunction logging notifies devs of Function execution handler activity. This is non-optional to begin with.
function-handler-logging-devxp.mov
function-localized-error-handling.mov
Requirements (place an
x
in each[ ]
)