Minor improvements #5
Conversation
TheColorDarkBlue
changed the title
TheColorDarkBlue
changed the title
|
Added a few comment, note and recommendation if you don't mind :) |
| ## Setup Instructions | ||
|
|
There was a problem hiding this comment.
Please consider to add requirements for the project (exact Node version, any kind of keys for webhook, etc)
| onEvent: async (event: ModMail, context: TriggerContext) => { | ||
| try { | ||
| if (!context) { | ||
| throw new Error('Context is probably undefined'); | ||
| throw new Error("Context is probably undefined"); |
There was a problem hiding this comment.
Please add logging here with context and details (what failed, why failed)
| } | ||
|
|
||
| await sendModMailToWebhook(event, context); | ||
| } catch (error: any) { | ||
| console.error('There was an error:', error.message); | ||
| console.error("There was an error:", error.message); |
There was a problem hiding this comment.
Please add meaningful error messages (why it failed, what failed)
Also, consider adding the entire error instead of the error.message to have stack trace and all possible details
| return; | ||
| } | ||
|
|
||
| const conversationId = event.conversationId ?? ''; | ||
| const actualConversationId = conversationId.replace('ModmailConversation_', ''); | ||
| const conversationId = event.conversationId ?? ""; |
There was a problem hiding this comment.
Please add a check for the conversationId to handle missing data or possible error(s).
|
|
||
| if (!lastMessage) { | ||
| console.error('No messages found'); | ||
| console.error("No messages found"); |
There was a problem hiding this comment.
Please consider to add a meaningful / informative error message with context and details.
Example:
console.error('No last message found. Terminating process. Message id: ', messageIds, '\n lastMessageId: ', lastMessageId, '\n conversationId: ', conversationId);
Such a verbose message gives you immediate context of what happens and details so you don't have to investigate or re-trigger something to have actual data.
| const authorProfileLink = `https://www.reddit.com/u/${authorName}`; | ||
|
|
||
| if (participatingAs === 'moderator' && !outgoing) { | ||
| console.log('Not sending outgoing messages to the webhook'); | ||
| if (participatingAs === "moderator" && !outgoing) { |
There was a problem hiding this comment.
Question: shouldn't need any check against "Unknown" ?
| }, | ||
| ], | ||
| }; | ||
| } else { | ||
| throw new Error('This webhook is neither from Slack nor Discord.'); | ||
| throw new Error("This webhook is neither from Slack nor Discord."); |
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| "Content-Type": "application/json", |
There was a problem hiding this comment.
Consider adding Accept... type header also (to enforce the response type), and consider adding the charset to the content-type
| // Let's handle the errors and log them | ||
| console.error('Error:', error.message); | ||
| // Handle errors and log them | ||
| console.error("Error:", error.message); |
There was a problem hiding this comment.
Please consider adding the entire error instead of just the error.message not to swallow/hide stack and information.
Also, I highly recommend to avoid redundant keyword stacking. .error Error. Rather give some human-readable message here, what kind of error you got and why (context & details)
example.test.tsContextimport from Devvit API