-
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
Fixes and polish for stable release #2128
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat-functions #2128 +/- ##
==================================================
+ Coverage 81.43% 81.58% +0.15%
==================================================
Files 19 19
Lines 1621 1640 +19
Branches 458 462 +4
==================================================
+ Hits 1320 1338 +18
Misses 194 194
- Partials 107 108 +1 ☔ View full report in Codecov by Sentry. |
ab80d52
to
9df1e68
Compare
9df1e68
to
f7966e7
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.
LGTM! Nice to see the stronger typings here 🙏 Left a few comments around imports but nothing blocking IMO
@@ -13,7 +13,7 @@ import { | |||
FunctionExecutedEvent, | |||
} from './types'; | |||
import processMiddleware from './middleware/process'; | |||
import { CustomFunctionInitializationError } from './errors'; | |||
import { CustomFunctionCompleteFailError, CustomFunctionCompleteSuccessError, CustomFunctionInitializationError } from './errors'; |
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.
We might want to just use the CustomFunctionInitializationError
error here to not confuse initialization and runtime errors 🤔
@@ -1088,6 +1093,7 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed> | |||
if (type === IncomingEventType.Action && context.functionExecutionId !== undefined) { | |||
listenerArgs.complete = CustomFunction.createFunctionComplete(context, client); | |||
listenerArgs.fail = CustomFunction.createFunctionFail(context, client); | |||
listenerArgs.inputs = context.functionInputs; |
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.
Just sharing as an option, free to ignore- all listener arguments could be created and assigned with Object.assign
if we wanted to reduce the number of CustomFunction
exports
@@ -1,7 +1,7 @@ | |||
import { WebClient } from '@slack/web-api'; | |||
import { Logger } from '@slack/logger'; | |||
import { StringIndexed } from './helpers'; | |||
import { SlackEventMiddlewareArgs } from './events'; | |||
import { FunctionInputs, SlackEventMiddlewareArgs } from './events'; |
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.
Could these be imported from ./functions
instead?
Summary
Final preparations and polish prior to cutting a stable release. 💅🏼
inputs
available by way ofContext
for interactivity events that are associated with a function's executionContext
interfacefunctionExecutionId
somehow goes missing when using a utilityfeat-functions
) PR feedback (TS fixes)action
payloadRequirements (place an
x
in each[ ]
)