disable slack webhooks if app is not configured #1153
Conversation
🦋 Changeset detectedLatest commit: 8e6a499 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
10 Ignored Deployments
|
| constructor( | ||
| private ownAppId: string, | ||
| private client: Pick<Client, "query" | "mutation">, | ||
| options?: { | ||
| WebhooksClient: WebhooksActivityClient; | ||
| }, | ||
| ) { | ||
| this.webhooksClient = options?.WebhooksClient ?? new WebhooksActivityClient(this.client); | ||
| } |
There was a problem hiding this comment.
Why do we construct WebhooksActivityClient in here? Even if you want to use the service with a mocked options.WebhookClient you still need to provide a mock for client, even though it's not used anywhere in the service itself - only for initialization
There was a problem hiding this comment.
Its dependency injection. If you don't provide options, it will automatically create instance. But for testing, you can create any mocked instance of WebhooksActivityClient.
I agree that this implementation should be improved, so graphql client is not needed. It should be hidden behind injected webhooksClient. But this is implementation copy-pasted from Search app.
I'm happy to fix this, but this should probably happen with extraction of this service to shared package and later implementing to every other app.
This PR is meant to quickly implement performance optimization, I will add ticket for it
| const isValidUrl = (url: string) => { | ||
| try { | ||
| new URL(url); | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
suggestion: I think this could be extracted into a separate utility, since the same snippet is used in apps/slack/src/pages/api/configuration.ts
Scope of the PR
Related issues
Checklist