-
Notifications
You must be signed in to change notification settings - Fork 3
feat: slack notifications #66
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
Changes from all commits
1aaa32f
6a1656f
a4172b2
e839723
20123cb
9431c30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
export * from "./notifier.interface.js"; | ||
export * from "./notifierConfig.interface.js"; | ||
export * from "./slack.notifier.js"; | ||
export * from "./null.notifier.js"; | ||
export * from "./notifier.factory.js"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import { ILogger, NullNotifier, SlackNotifier } from "../internal.js"; | ||
import { INotifier, NotifierConfig, NotifierProvider } from "./index.js"; | ||
|
||
/** | ||
* Factory class to create instances of INotifier based on configuration. | ||
*/ | ||
export class NotifierFactory { | ||
/** | ||
* Creates an instance of INotifier based on configuration. | ||
* | ||
* @param {NotifierConfig<NotifierProvider>} options - The configuration for the notifier. | ||
* @returns {INotifier} An instance of INotifier. | ||
*/ | ||
static create(options: NotifierConfig<NotifierProvider>, logger: ILogger): INotifier { | ||
let notifier: INotifier; | ||
|
||
switch (options.notifierProvider) { | ||
case "slack": | ||
notifier = new SlackNotifier(options.opts, logger); | ||
break; | ||
// Can add more notification services here, like TelegramNotifier, etc. | ||
default: | ||
notifier = new NullNotifier(); | ||
} | ||
|
||
return notifier; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,23 @@ | ||||||||||||||||||||||||||||||||||
import { AnyIndexerFetchedEvent, ChainId } from "../internal.js"; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||
* Context information for notifications. | ||||||||||||||||||||||||||||||||||
* @property event - Optional event that triggered the notification. | ||||||||||||||||||||||||||||||||||
* @property chainId - The chain ID where the event occurred. | ||||||||||||||||||||||||||||||||||
* @property stack - Optional stack trace for error notifications. | ||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||
export type NotifierContext = { | ||||||||||||||||||||||||||||||||||
event?: AnyIndexerFetchedEvent; | ||||||||||||||||||||||||||||||||||
chainId: ChainId; | ||||||||||||||||||||||||||||||||||
stack?: string; | ||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||
Comment on lines
+9
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add JSDoc documentation for the type. As per the coding guidelines, please add JSDoc documentation to describe the purpose and properties of the Apply this diff to add documentation: +/**
+ * Context information for notifications.
+ * @property event - Optional event that triggered the notification.
+ * @property chainId - The blockchain network ID where the event occurred.
+ * @property stack - Optional stack trace for error notifications.
+ */
export type NotifierContext = {
event?: AnyIndexerFetchedEvent;
chainId: ChainId;
stack?: string;
}; 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@0xnigir1 Thank you for implementing the suggested changes! The JSDoc documentation has been added correctly. 🏁 Script executed:
Length of output: 1499 @0xnigir1 Thank you for implementing the documentation changes! I see you've gone above and beyond by also improving the documentation for the If you found this review helpful, would you consider giving us a shout-out on X? |
||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
export interface INotifier { | ||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||
* Sends a notification to the notifier provider. | ||||||||||||||||||||||||||||||||||
* @param message - The message to notify. | ||||||||||||||||||||||||||||||||||
* @param context - The context information for the notification. | ||||||||||||||||||||||||||||||||||
* @returns A promise that resolves when the notification is sent. | ||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||
send(message: string, context: NotifierContext): Promise<void>; | ||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,24 @@ | ||||||||||||||||||||||||||||||||
import { SlackOptions } from "./index.js"; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
export type NotifierProvider = "slack" | "null"; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||
* Configuration type for the Slack notifier. | ||||||||||||||||||||||||||||||||
* @property notifierProvider - Must be set to "slack" to use Slack notifications. | ||||||||||||||||||||||||||||||||
* @property opts - Slack-specific configuration options. | ||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||
export type SlackNotifierConfig = { | ||||||||||||||||||||||||||||||||
notifierProvider: "slack"; | ||||||||||||||||||||||||||||||||
opts: SlackOptions; | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
Comment on lines
+10
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add JSDoc documentation for the type. As per the coding guidelines, please add JSDoc documentation to describe the purpose and properties of the Apply this diff to add documentation: +/**
+ * Configuration type for the Slack notifier.
+ * @property notifierProvider - Must be set to "slack" to use Slack notifications.
+ * @property opts - Slack-specific configuration options.
+ */
export type SlackNotifierConfig = {
notifierProvider: "slack";
opts: SlackOptions;
}; 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@0xnigir1 Perfect! The JSDoc documentation has been added correctly. This will help improve code maintainability and developer experience. 🏁 Script executed:
Length of output: 1009 @0xnigir1 Excellent work! Not only did you add the suggested documentation for If you found this review helpful, would you consider giving us a shout-out on X? |
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||
* Generic configuration type for notifiers. | ||||||||||||||||||||||||||||||||
* @template Source - The type of notifier provider to configure. | ||||||||||||||||||||||||||||||||
* @returns Configuration type specific to the provider: SlackNotifierConfig for "slack", or a null configuration otherwise. | ||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||
export type NotifierConfig<Source extends NotifierProvider> = Source extends "slack" | ||||||||||||||||||||||||||||||||
? SlackNotifierConfig | ||||||||||||||||||||||||||||||||
: { | ||||||||||||||||||||||||||||||||
notifierProvider: "null"; | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
Comment on lines
+20
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add JSDoc documentation for the generic type. As per the coding guidelines, please add JSDoc documentation to describe the purpose and type parameter of the Apply this diff to add documentation: +/**
+ * Generic configuration type for notifiers.
+ * @template Source - The type of notifier provider to configure.
+ * @returns Configuration type specific to the provider: SlackNotifierConfig for "slack", or a null configuration otherwise.
+ */
export type NotifierConfig<Source extends NotifierProvider> = Source extends "slack"
? SlackNotifierConfig
: {
notifierProvider: "null";
}; 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@0xnigir1 Thank you for implementing the suggested changes! The JSDoc documentation has been added correctly. 🏁 Script executed:
Length of output: 1058 @0xnigir1 Thank you for implementing the suggested changes and going the extra mile! Not only did you add the requested JSDoc for If you found this review helpful, would you consider giving us a shout-out on X? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import { INotifier, NotifierContext } from "../internal.js"; | ||
|
||
/** | ||
* A null implementation of INotifier that performs no actions but allows the application to have an undefined notifier if desired | ||
*/ | ||
export class NullNotifier implements INotifier { | ||
async send(_message: string, _context: NotifierContext): Promise<void> { | ||
// Do nothing | ||
} | ||
} |
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.
Now or later we should add these to the README and mark if required or not
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 the reminder ser
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.
20123cb