-
Notifications
You must be signed in to change notification settings - Fork 394
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
bolt v4 #2254
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2254 +/- ##
==========================================
+ Coverage 87.78% 87.81% +0.02%
==========================================
Files 19 20 +1
Lines 1646 1649 +3
Branches 464 464
==========================================
+ Hits 1445 1448 +3
Misses 194 194
Partials 7 7 ☔ View full report in Codecov by Sentry. |
// Filter out any non-actions | ||
if (action === undefined) { | ||
return; | ||
export const onlyActions: Middleware<AnyMiddlewareArgs> = async (args) => { |
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.
Breaking change: no longer need to call this as onlyActions()
, instead, it is a middleware itself and can be simply passed as onlyActions
. The wrapping closure within was unnecessary. This lines it up with other, non-parameterized middleware in this module.
if (action === undefined) { | ||
return; | ||
export const onlyActions: Middleware<AnyMiddlewareArgs> = async (args) => { | ||
if ('action' in args && args.action) { |
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.
Instead of casting, in TS we should be using more of 'property' in object
style checks and let TS do the type narrowing for us.
// Filter out any non-shortcuts | ||
if (shortcut === undefined) { | ||
return; | ||
export const onlyShortcuts: Middleware<AnyMiddlewareArgs> = async (args) => { |
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.
Another breaking change: this is a middleware, no need for extra internal closure.
// Filter out any non-commands | ||
if (command === undefined) { | ||
return; | ||
export const onlyCommands: Middleware<AnyMiddlewareArgs> = async (args) => { |
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.
Same deal! This is middleware, remove internal closure.
// Filter out any non-options requests | ||
if (options === undefined) { | ||
return; | ||
export const onlyOptions: Middleware<AnyMiddlewareArgs> = async (args) => { |
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.
Broken record: middleware, remove internal closure.
// Filter out any non-events | ||
if (event === undefined) { | ||
return; | ||
export const onlyEvents: Middleware<AnyMiddlewareArgs> = async (args) => { |
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.
You guessed it: middleware, 🔪 closure
// Filter out anything that doesn't have a view | ||
if (view === undefined) { | ||
return; | ||
export const onlyViewActions: Middleware<AnyMiddlewareArgs> = async (args) => { |
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.
Samesame
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.
💡 🧠 Big fan of it all!
return; | ||
} | ||
} | ||
export const ignoreSelf: Middleware<AnyMiddlewareArgs> = async (args) => { |
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.
Same: 🔪 internal closure since we don't use any of the outer function's parameters.
if (isMessageEventArgs(args)) { | ||
const { message } = args; | ||
// Look for an event that is identified as a bot message from the same bot ID as this app, and return to skip | ||
if (message.subtype === 'bot_message' && message.bot_id === botId) { |
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.
The combination of cleaning up the middleware types and use of type predicates lets us clean up the type casting and simplify the logic in this method ❤️
| GlobalShortcut | ||
| InteractiveMessageSuggestion | ||
| DialogSuggestion; | ||
export const directMention: Middleware<SlackEventMiddlewareArgs<'message'>> = async ({ message, context, next }) => { |
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.
🔪 closure, straight middleware
src/receivers/AwsLambdaReceiver.ts
Outdated
@@ -10,15 +10,15 @@ import { StringIndexed } from '../types/helpers'; | |||
|
|||
export interface AwsEvent { | |||
body: string | null; | |||
headers: any; | |||
headers: Record<string, string>; |
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.
Breaking change: HTTP request headers are string:string maps
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.
Let's follow this type def: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/aws-lambda/trigger/api-gateway-proxy.d.ts It seems the value in the data can be undefined
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.
Resolving #2272 as well is a plus
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 looked at this more closely. Here is my assessment - let me know if you agree / if I should proceed / how to adjust:
- The v1 payload definition is here and it looks like:
{ // Shared properties with v2: body: string | null; headers: APIGatewayProxyEventHeaders; isBase64Encoded: boolean; pathParameters: APIGatewayProxyEventPathParameters | null; queryStringParameters: APIGatewayProxyEventQueryStringParameters | null; requestContext: APIGatewayEventRequestContextWithAuthorizer<TAuthorizerContext>; stageVariables: APIGatewayProxyEventStageVariables | null; // v1 only properties: multiValueHeaders: APIGatewayProxyEventMultiValueHeaders; httpMethod: string; path: string; multiValueQueryStringParameters: APIGatewayProxyEventMultiValueQueryStringParameters | null; resource: string; }
- The v2 payload definition is here and it looks like:
{ // Shared properties with v1: body?: string; headers: APIGatewayProxyEventHeaders; isBase64Encoded: boolean; pathParameters?: APIGatewayProxyEventPathParameters; queryStringParameters?: APIGatewayProxyEventQueryStringParameters; requestContext: TRequestContext; stageVariables?: APIGatewayProxyEventStageVariables; // v2 only properties: version: string; routeKey: string; rawPath: string; rawQueryString: string; cookies?: string[]; }
I took a pass at typing this as a union of V1Payload | V2Payload
- as you suggested, see the compare view with this branch here - but because of the incompatible types for some of the same properties (e.g. body
is string | null
in v1 but string | undefined
in v2) it may lead to TypeScript compilation problems in userland code, depending on how userland code uses these properties. May be problematic, not sure, though it shouldn't be difficult; it should be as simple as using the following in user code:
if ('version' in payload) {
// payload is now v2
} else {
// payload is now v1
}
You can also see in the compare view that it does require even minor modifications to the receiver code; that is, references to awsEvent.path
need to be narrowed first into either v1 (where path
is available) or v2 (where instead rawPath
should be used). I think this is actually safer code: it correctly and explicitly deals with either payload more appropriately, in my opinion. So I am in favour of this change, but, what do you think @seratch ? If we agree on this change, I would definitely add a section to the migration guide to cover this change and document users how to migrate their code in case they do need to touch the AwsEvent
interface. I think this could affect userland code if users define their own customPropertiesExtractor
or invalidSignatureHandler
methods, since those methods are passed an AwsEvent
argument.
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've created a draft PR into this PR to be able to more specifically discuss this one change here: #2277
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 for quickly resolving this! Yes, I agree that this type of change is fine when we release a new major version.
src/receivers/AwsLambdaReceiver.ts
Outdated
queryStringParameters: any | null; | ||
multiValueQueryStringParameters: any | null; | ||
stageVariables: any | null; | ||
pathParameters: any; |
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.
any
already includes null
, so can clean this up.
src/receivers/AwsLambdaReceiver.ts
Outdated
multiValueQueryStringParameters: any | null; | ||
stageVariables: any | null; | ||
pathParameters: any; | ||
queryStringParameters: Record<string, string>; |
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.
Same as headers, query string parameters are string:string maps.
@@ -529,7 +529,7 @@ describe('ExpressReceiver', function () { | |||
// Act | |||
const req = { body: { }, url: 'http://localhost/slack/oauth_redirect', method: 'GET' } as Request; | |||
const resp = { send: () => { } } as Response; | |||
(receiver.router as any).handle(req, resp); | |||
(receiver.router as any).handle(req, resp, () => {}); |
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.
A change that was required as part of the move from express v4 -> v5
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.
Great work; Left a few comments!
src/receivers/AwsLambdaReceiver.ts
Outdated
@@ -10,15 +10,15 @@ import { StringIndexed } from '../types/helpers'; | |||
|
|||
export interface AwsEvent { | |||
body: string | null; | |||
headers: any; | |||
headers: Record<string, string>; |
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.
Let's follow this type def: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/aws-lambda/trigger/api-gateway-proxy.d.ts It seems the value in the data can be undefined
src/receivers/AwsLambdaReceiver.ts
Outdated
multiValueQueryStringParameters: any | null; | ||
stageVariables: any | null; | ||
pathParameters: any; | ||
queryStringParameters: Record<string, string>; |
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.
According to https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/aws-lambda/trigger/api-gateway-proxy.d.ts, this can be either object or null
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.
Great call to follow that project; what if we used it as a dependency instead of maintaining our own types?
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.
Indeed it's worth considering. Perhaps as an optional dependency (like we plan for express)?
src/receivers/AwsLambdaReceiver.ts
Outdated
@@ -10,15 +10,15 @@ import { StringIndexed } from '../types/helpers'; | |||
|
|||
export interface AwsEvent { | |||
body: string | null; | |||
headers: any; | |||
headers: Record<string, string>; |
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.
Resolving #2272 as well is a plus
@@ -1,25 +1,17 @@ | |||
{ |
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 haven't checked this is backward-compatible yet, but running npm pack
with both old and new settings and verifying the diff (or any technique like that) would be worth doing (you might have already done this tho!)
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 did add example integration tests in the test PR (see here); the application TypeScript seems to compile fine when integrated with this PR.
I will do a manual testing pass with the other well-known sample bolt-js apps we have to validate everything works as desired. I don't think the emitted JavaScript should be different, since it is still set to emit commonjs
(just like before) and module resolution is still node
(just like before), but I will validate manually. And if there are any other sample applications we should integrate with, let me know and I can add it to the CI (at least compilation-level validation with TypeScript projects is an easy integration test to run).
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 works fine with the following samples:
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 for double-checking this! 🙇
…ments. fix up getting started typescript example
…ore. the ts-ignore comment is worse than using fallthrough.
… type assertions.
…d_teams) from event payload envelope interface
…e invoked as a method to return middleware; instead, they are directly middleware, like other built-in middleware functions.
…e `message` parameter. THIS FIXES SO MUCH! shouts to jcalz on stackoverflow
…ntMiddlewareArgs when event contains channel information.
…ionMiddlewareArgs when a non-dialog or step from app action
…rtcutMiddlewareArgs when a non-dialog or step from app action. also TODOs
… flat map of strings to strings
…inder removal todos for eventual removal
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.
Awesome and wonderful changes all throughout! I tested this with some other apps and am finding things build well 🙏 ✨
Most comments I left might be noise and excitement, but I did leave a few thoughts on the @slack/types
export that I'm open to discussing more! 🚀
I'll also continue to test, but am noticing something strange happening with @slack/socket-mode
and incoming events - going to open a separate issue for this though 👀
From the look of things, the migration guide is also looking to be a smooth process too - super nice 👏 👏
// Filter out anything that doesn't have a view | ||
if (view === undefined) { | ||
return; | ||
export const onlyViewActions: Middleware<AnyMiddlewareArgs> = async (args) => { |
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.
💡 🧠 Big fan of it all!
@@ -119,7 +119,7 @@ app.action<BlockAction>({ action_id: 'select_user', block_id: 'assign_ticket' }, | |||
await client.reactions.add({ | |||
name: 'white_check_mark', | |||
timestamp: body.message?.ts, | |||
channel: body.channel?.id, | |||
channel: body.channel!.id, // if the body has a message, we know it has a channel, too. |
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 adding body.channel
to the if
above remove this check and ?
@@ -2,21 +2,24 @@ import { SlackEvent } from '@slack/types'; | |||
import { StringIndexed } from '../helpers'; | |||
import { SayFn } from '../utilities'; | |||
|
|||
// TODO: for backwards compatibility; remove at next major (breaking change) | |||
export * from '@slack/types'; |
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.
Should we remove re-exporting of
@slack/types
? Instead, developers could add a separate dependency on it for their own use. That might be nice, but also it may be nice to re-export the same@slack/types
version that is bundled in bolt.
🤔 Hmmmm... I'm thinking it makes sense to keep the export to avoid possible version drifts between the latest
of each package, though this might not be a concern between minor or patch bumps?
We might also need to remove this from src/index.ts
too, but I'm super curious what you think about a named export instead?
export * as types from '@slack/types';
This could make working with Block Kit more clear IMO when using typeahead to find the blocks or similar types of exploration 🔍
import bolt from "@slack/bolt";
const header: bolt.types.HeaderBlock = {
type: "header",
text: {
type: "plain_text",
text: "Greetings traveler!",
},
};
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.
Sounds good re: keeping the types export. On this topic, now that I think about it, we actually do a similar thing in deno-slack-sdk: it includes the deno-slack-api project, and initially we kept them separate and had users import both. This turns out to be a pain for developers, and I regret that decision. So, let's keep the types export.
And I like your idea to do a named export; will play around with that.
/** Using type parameter T (generic), can distribute the Omit over a union set. */ | ||
type DistributiveOmit<T, K extends PropertyKey> = T extends any | ||
? Omit<T, K> | ||
: never; |
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.
This might be nice to include with the src/types/helpers.ts
🐙 But no blocker!
"@types/express": "^4.16.1", | ||
"@types/promise.allsettled": "^1.0.3", | ||
"@types/tsscmp": "^1.0.0", | ||
"@slack/web-api": "^7", |
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.
This is huge!! 🙏 ✨ 🚀
Summary
PR (WIP) of what is being discussed as supported for a bolt-js v4 release. Breaking changes are planned for this release!
The tests are failing because of linter and a few other issues, however, #2259 which is intended to be merged into this PR, has all the tests passing. The reason for this split is to make reviewing the major version changes easier on their own in this PR, separate from test utility/linter/reorganization changes.
Open Questions
Should we remove re-exporting ofYes let's keep the exports, but try a named export to clearly delineate the types separately from everything else.@slack/types
? Instead, developers could add a separate dependency on it for their own use. That might be nice, but also it may be nice to re-export the same@slack/types
version that is bundled in bolt. Thoughts?Breaking Changes
Middleware Type Changes
This one I'm particularly excited about! In bolt we have a set of
Slack*MiddlewareArgs
types: for events, shortcuts, commands, and so on. They 'wrap' the underlying event payloads with additional middleware-relevant bits like anext()
method, acontext
object for devs to augment, and so on.Many of these types, for example the
SlackEventMiddlewareArgs
type, previously used a conditional to sometimes define particular additional helper utilities on the middleware arguments. For example, thesay
utility, or tacking on a conveniencemessage
property for message-event-related payloads. This was problematic in practice in TypeScript situations, not just internally (this change fixes #2135) within the bolt codebase but for developers as well: when the payload was not of a type that required the extra utility, these properties would be required to exist on the middleware arguments but have a value ofundefined
. Those of us trying to build generic middleware utilities would have to deal with TS compilation errors and needing to liberally type-cast to avoid these conditional mismatches withundefined
.Instead, these
MiddlewareArgs
types now conditionally create a type intersection when appropriate in order to provide this conditional-utility-extension mechanism. In practice that looks something like:With the above, now when a message payload is wrapped up into middleware arguments, it will contain an appropriate
message
property, whereas a non-message payload will be intersected withunknown
- effectively a type "noop." No more e.g.say: undefined
ormessage: undefined
to deal with!Other Breaking Changes
express
to v4->v5;ExpressReceiver
users will be exposed to express v4 -> v5 breaking changes@slack/socket-mode
v2;SocketModeReceiver
users who have attached custom event listeners to the publicsocketModeClient
directly should read the v1 -> v2 migration guide in case the major upgrade could affect them@slack/web-api
v7; all users should read the web-api v6->v7 migration guide to see what the scope of breaking changes theclient
within listeners is affected byKnownKeys
@slack/types
now exist under a named export.SocketModeFunctions
class that had a single static method on it and instead directly exposed thedefaultProcessEventErrorHandler
method from it.ignoreSelf
anddirectMention
now no longer must be invoked as a method in order to return middleware; instead they are middleware to be used directly. this lines up the API for these built-in middlewares to match the other builtins.AwsEvent
interface now models event payloads a bit differently; thanks to @seratch's astute review, some additional issues were identified and there's a secondary PR to review for this here: Bolt v4 addendum: stricter typing of AWS API Gateway payloads #2277OptionsRequest
interfaceauthed_users
andauthed_teams
from event payload enveloperender-html-for-install-path
moduleverify
andVerifyOptions
from theverify-request
modulesrc/receivers/http-utils.ts
moduleNon-breaking Changes
tsconfig.json
switch
statements, removed that fromtsconfig.json
; the alternative of using@ts-ignore
comments in places where we do is dangerous, as non-fallthrough-in-switch type errors are also ignored from itmap
)processEvent
raw-body
to v3@slack/oauth
to v3promise.allsettled
since that is natively supported in node since v14@types/tsscmp
to dev dependencies since that is not exposed to developersTODO
Post-release TODO