test: confirm correct app.function handler handling #2139
Conversation
* ensure custom function middleware uses the configured token * ensure action handlers have middleware for function executions * ensure listener middleware is honored in function handlers * ensure custom function middleware arguments call correct apis * ensure custom function handlers are setup with valid arguments * document usage of attributes and handlers inline with jsdoc * refactor logic and types into custom function specific files
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## pre-stable-fixes #2139 +/- ##
====================================================
+ Coverage 80.87% 82.49% +1.61%
====================================================
Files 19 20 +1
Lines 1637 1617 -20
Branches 461 461
====================================================
+ Hits 1324 1334 +10
+ Misses 205 179 -26
+ Partials 108 104 -4 ☔ View full report in Codecov by Sentry. |
| * Overrides for spying on mocks are below this comment | ||
| */ | ||
|
|
||
| async function importApp( |
There was a problem hiding this comment.
This is nice 💯!
Any chance we could bring it out of this file so the pattern can be reused by other tests? No worries if you don't have time
| @@ -527,10 +536,26 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |||
| } | |||
|
|
|||
| /** | |||
| * Register CustomFunction middleware | |||
| */ | |||
| * Custom function listener and middleware that executes for a matching function callbackId. | |||
src/CustomFunction.ts
Outdated
| * @param context - the incoming payload context. | ||
| * @link https://github.com/slackapi/bolt-js/pull/2026#discussion_r1467123047 | ||
| */ | ||
| private static selectToken(context: Context): string | undefined { |
There was a problem hiding this comment.
I think this logic is duplicated in the App.ts 🤔
Might be a good idea to consolidate it
There was a problem hiding this comment.
Great call once more! Had to think a bit about where this might make sense - exporting from App.ts into CustomFunction.ts felt strange - and ended up creating middleware/context.ts for similar context operations. Open to adjustments though ✨
Co-authored-by: William Bergamin <william.bergamin.coen@gmail.com>
|
@WilliamBergamin appreciate all of those suggestions and the review. The code gets pushed in a better direction with each. I'm feeling solid about these changes and am interested in tagging a new |
|
Update: Holding off on tagging another beta with these changes - will mark it as draft for following up sometime soon. |
| 'retryNum', | ||
| 'retryReason', | ||
| ...Array<keyof CustomFunctionContext>(), |
There was a problem hiding this comment.
What do you think about explicitly defining fields?
I think it can make the code clearer for future maintainers and prevent a field from being accidentally added through an inherited object 🤔
There was a problem hiding this comment.
I'm partial to both approaches! The magic is nice to avoid forgetting to add it to context and needing to hunt for these fields, but magic is a dangerous art 😳
9df1e68 to
f7966e7
Compare
Summary
This PR tests the implementation of
app.functionhandlers across a few cases with some refactors and fixes found along the way.Changes that extend those found in #2026 and #2128:
app.functiontypesdirectoryPreview
Experiment with an app that uses listener middleware and functions with actions after checking out and building this branch:
$ slack create baker -t zimeg/slacks -b js.bolt.cookies $ cd baker $ npm install ../../path/to/bolt-js $ slack runThen add the
cookiesfunction as a step to a workflow.Reviewers
The tests in
src/App-custom-function.spec.tscover the use cases I could think of! Please let me know of others that should be considered 🙏Notes
Will follow up with updates to https://slack.dev in another PR to avoid cluttering this diff too much!
Requirements