Recently, I wrote a simple Slack bot that queries an API for feedback on our products, then posts that feedback into a specified channel. The bot also allows users to vote on whether or not a piece of feedback is actionable for them (ie: "there's a typo in this lesson" vs. "ok" or "no comment").
Since this was a "Hack Day" project, the initial implementation lived up to its name and was very hacky - votes were not stored by the server; users could vote as many times as they wanted. The voting was handled by modifying the string that came in with a POST /handle-vote request (Slack as a persistence layer 😂).
// text => 'Yes: 0 No: 0'
// value => 'yes' || 'no
function updateAttachmentText(text, value) {
votes = text.split(' ')
if (value === 'no') {
votes[3] = parseInt(votes[3]) + 1
} else if (value === 'yes') {
votes[1] = parseInt(votes[1]) + 1
}
return votes.join(' ')
}
const updated = updateAttachmentText('Yes: 0 No: 0', 'yes')
// => 'Yes: 1 No: 0'
This hacky little bot turned out to be quite useful for our product team - but I knew its dark, terrible secrets and decided to write a more robust version that used Redis for storage of vote data - it would store voters' Slack user ids and prevent one user from voting multiple times.
The bot itself uses a cron job to post new feedback to the channel. While upgrading, I added a step to that script to create a new "blank" record with that feedback's ID.
const initialVotes = { votes: { yes: [], no: [] } }
redisClient.store(id, JSON.stringify(initialVotes))
Once a user hits a button, the server receives the request, looks up the piece of feedback by its ID, adds the user's id to the correct list ('yes' or 'no'), then saves it back to the Redis store after performing some logic to ensure users can only vote once, and only one way.
The issue here is with messages from the original bot - these bits of feedback don't have records associated with their IDs in our application; so the following code would fail if the user clicked on a vote button:
// Imagine our Redis client setup...
class Redis {
// setup, etc
fetch(key) {
return new Promise((resolve, reject) => {
this.client.get(key, (err, response) => {
if (err || !response) {
return reject('Unable to find result for', key, err)
}
return resolve(response)
})
})
}
}
// ... and a Vote loading class...
class Vote {
constructor(id, redisClient) {
this.id = id
this.redisClient = redisClient
}
loadVote() {
return this.redisClient.fetch(this.id)
.then(voteData => JSON.parse(voteData))
}
}
const vote = new Vote(someId, someRedisClient)
vote.loadVote().then((vote) => incrementCounterAndSave(vote))
// Uncaught rejection :(
Initially I thought this would be an annoying problem where I'd need conditional logic to handle the record not existing somewhere in my server code. Looking at the Vote class's code itself though, reveals a neater option:
class Vote {
// ...
loadVote() {
return this.redisClient.fetch(this.id)
.then(voteData => JSON.parse(voteData))
.catch((err) => {
console.log('Encountered an error, returning placeholder data:', err)
return { votes: { yes: [], no: [] } }
})
}
}
const vote = new Vote(someId, someRedisClient)
vote.loadVote()
.then((vote) => {
console.log(vote)
incrementCounterAndSave(vote)
})
// Encountered an error, returning placeholder data:
// 'Unable to find result for someId (error here)
// { votes: { yes: [], no: [] } }
I figured I'd write this up since, while I've worked with Promises for a while now, this concept wasn't my first instinct: I didn't think to use catch anywhere but at the very end of my chain of then calls.
Here's some code you can play around with in the console that demonstrates this pretty simply!
class DataFetcher {
constructor() {
this.count = 0
}
fetch() {
this.count += 1
return new Promise((resolve, reject) => {
// Cause this Promise to be rejected/fail every other time the fetch function is called.
this.count % 2 === 0 ? resolve('data from DB!') : reject('data not found')
})
}
}
const client = new DataFetcher()
const getData = () => {
return client.fetch()
.then((res) => {
return res
})
.catch(err => {
return 'placeholder data'
})
}
getData.then(console.log) // placeholder data
getData.then(console.log) //data from DB!
As a side note, you could totally write this code in a less nested (and arguably more readable) way using async/await - I don't feel super strongly about it either way, so I just went with Promises.
Happy coding!
Top comments (5)
Nice article, thanks for sharing!
Have you considered, however, how to handle programming errors (or any error not related to the data you are loading)? In your
loadVotemethod, for example, if you misspellJSON.parsetoJSON.pasre, thecatchwill swallow the error and start sending default data to everybody.For sure! I personally really like Typescript for minimizing those kinds of errors - this code was actually originally written with type annotations, but I removed them for the article because I thought they might be confusing.
But yes, you do make a good point - for something like a misspelling, hopefully I've written unit tests to catch that! In production code, I'd have some kind of error monitoring library like Honeybadger or Bugsnag to let me know when an error occurs or reaches a certain threshold. This code definitely still has the potential to run into errors, but that way I'll at least be alerted.
Yeah, I'm a bit weary of Promises because they catch all and any errors that might happen in the rejection branch and it's hard to discriminate whether it's an error that should or shouldn't happen. That's why I switched to using Futures a while ago. You still could map the rejection branch to a default value like you suggest, but it won't mask errors that it shouldn't handle.
I was curious to see what kind of difficulties you had encountered with that, hence my question :)
Oh, my co-worker is super into futures - he keeps pressing me to try them out. They look really interesting! Yeah, that part of Promises gets annoying; we've been having some discussions about when/what/where to catch and what to do then.
That makes sense