Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

Use async function instead of promise (#325) #404

Merged
merged 3 commits into from
Aug 3, 2017
Merged

Use async function instead of promise (#325) #404

merged 3 commits into from
Aug 3, 2017

Conversation

weihanglo
Copy link
Contributor

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?

Copy link
Contributor

@dannycoates dannycoates left a 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs an await

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 596ad87!

@dannycoates
Copy link
Contributor

dannycoates commented Aug 3, 2017

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.

@dannycoates
Copy link
Contributor

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);
Copy link
Collaborator

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.

Copy link
Contributor Author

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 => {
Copy link
Collaborator

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.

@dannycoates dannycoates merged commit 5c793dc into mozilla:master Aug 3, 2017
@dannycoates
Copy link
Contributor

Thanks again, @weihanglo!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants