-
Notifications
You must be signed in to change notification settings - Fork 74
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
[CHAT-1467] Expose webhook_events field #792
Conversation
Size Change: 0 B Total Size: 252 kB ℹ️ View Unchanged
|
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.
It would also be nice to squash the commits here if possible, even though I see we haven't been super strict with the commit history so far.
@@ -1542,6 +1543,7 @@ export type AppSettings = { | |||
sqs_key?: string; | |||
sqs_secret?: string; | |||
sqs_url?: string; | |||
webhook_events?: Array<string> | 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.
Are the events the available events the ones listed here: https://getstream.io/chat/docs/other-rest/webhook_events/?language=&q=webhook#webhook-event-types. If yes maybe we could make this type a bit more strict, like:
type WebhookEvents = 'message.new' | 'message.read' | ... | 'user.unflagged';
{
// ...
webhook_events?: Array<WebhookEvents> | null;
// ...
}
And use it on the response as well. Does it make sense?
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.
good improvement 👍
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.
Those are events listed there + custom events
We don't have any validation for that column on a backend, it should be "array of strings or null", because we don't have dedicated sorta type
for custom events
So i'd say that your suggestion makes perfect sense but not exactly here
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.
out of scope of this PR, but the suggestion is still a good improvement, it can be:
webhook_events?: 'message.new' | 'message.read' | ... | 'user.unflagged' | string | 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.
it looks good, but that "string" in the end drops benefits of "message.new| message.read" to near zero..
but i'm not ts
guy, if you folks want i can include it, no problem =)
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're right, this is not helpful for static type checking(could be with the help of Type Guards) but mostly for auto-completion and ref check, pretty common in TS world, however let's keep this change outside of this PR
@vicnicius I've limited the merge options, would be good to set up a workflow for changelog as well |
Very true, good reminder. I'll draft something regarding commits and changelog and share it somewhere for us to discuss. |
We're extending
updateApp
API with new property, called "webhook_events". It is used to filter events sent via webhooks/sqs.null
will reset this filter to default state;GetApp
response will contain default event types set