-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Use async function instead of promise (#325) #404
Conversation
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.
this looks great! just the one change...
server/server.js
Outdated
try { | ||
const filename = await storage.filename(id); | ||
const contentLength = await storage.length(id); | ||
const timeToExpiry = storage.ttl(id); |
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.
this needs an await
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.
Done 596ad87!
As you've discovered converting storage.js is going to be more complicated. Let's do that in a separate PR. The easiest strategy is probably to add bluebird, as recommended in the node_redis readme. We'll also probably need to adjust the code a bit if that changes the behavior of rejecting, i.e. change some try/catches into checking result values. |
Thanks @weihanglo! I'll verify this and merge it in the morning :) |
server/server.js
Outdated
}) | ||
.catch(err => res.sendStatus(404)); | ||
try { | ||
const err = await storage.delete(id, delete_token); |
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.
Super minor nit, but I think the indenting is slightly off here. Not sure if you want to try risking running npm run format
and running the prettier formatter against the codebase.
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.
Sorry I totally forgot it! Maybe we should add pre-commit or pre-push hook to prevent these kind of mistake.
.catch(err => { | ||
log.info('DeleteError:', newId); | ||
}); | ||
req.on('close', async err => { |
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.
Unrelated to PR, but I just noticed that the .on('close')
may return an err
response here (apparently), which we aren't checking, but then we're overwriting the err
value below on line 266. 🤷♀️
Behavior looks the same as it was previously, but there may be dragons here. Very small, uninteresting dragons.
Thanks again, @weihanglo! |
Changed all promises into async functions for express.
I have attempted to promsify all Redis client methods as #64 described. It failed but I have no idea why, and I cannot log directly in storage.js. Any help?