perf(potato): reorganize promises #19
Conversation
JonasBa
force-pushed
the
jb/perf/gib-potato
branch
from
8a202ca to
8b58011
Compare
| return result["user"]["real_name"]; | ||
| } catch (error) { | ||
| // Maybe do some proper error handling here | ||
| console.error(error); |
There was a problem hiding this comment.
I've removed these as uncaught exceptions are already going to be picked up by sentry.
| console.error(error); | ||
| Sentry.captureException(error); | ||
|
|
||
| return ""; |
There was a problem hiding this comment.
This would return an empty id in case of error and we were not checking if the id is falsy downstream. We'll see the crash in Sentry if it happens as opposed to silently hiding it. My guess is that this would not happen unless there is an intermittent problem with the slack query in which case we would still want to error.
| return uid; | ||
| } else { | ||
| return user["id"]; | ||
| if(user){ |
| // If the user is not found in the Database add it to the Database | ||
| return getUserNameBySlackId(slackId).then(async (fullName) => { | ||
| const userId = uuid() | ||
| const imageURL = (await app.client.users.profile.get({ user: slackId }))[ |
There was a problem hiding this comment.
only resolve the image if we are inserting a user
| // Check that there are potatos left to give for the sender (sender ids) | ||
| const senderDBId = await getUserDbIdOrCreateUser(senderSlackId); |
There was a problem hiding this comment.
Moved down in case we early return from any of the above statements and this is unused
| @@ -219,17 +211,12 @@ async function givePotato({user, text, channel, ts}) { | |||
| } | |||
|
|
|||
| // These will be our DB ids for the people that where mentioned | |||
| let userDBIds = []; | |||
|
|
|||
| for (const userSlackId of receiverSlackIds) { | |||
There was a problem hiding this comment.
This was executing each promise sequentially, we can do it in parallel.
|
|
||
| // Add the message's to the DB | ||
| userDBIds.forEach(async (user) => { |
There was a problem hiding this comment.
I dont think this was doing what we were expecting it to do... forEach here will return asap and not after all of the promises are awaited https://stackoverflow.com/questions/37576685/using-async-await-with-a-foreach-loop
JonasBa
changed the title
|
@JonasBa Thanks! Might take a while until this gets merged, I have to test it which is a bit involved 😬 |
|
Np, @cleptric, I'm fairly confident this should be fine, but better to be safe than sorry :D |
Moved some promises around and used Promise.all so we can resolve them in parallel.