Refactor to async-await (#521) #973

siddhant1 · 2019-03-22T16:24:18Z

I have verified that this pull request:

has no linting errors (npm run lint)
[ x] is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
[x ] is descriptively named and links to an issue number, i.e. Fixes #521

siddhant1 · 2019-03-22T16:26:37Z

@catarak I am working on working on all the controllers , Should I make separate prs or add commits here?

siddhant1 · 2019-03-22T16:57:47Z

I am currently having some linting error because of my global prettier settings, I will fix them at the end.

siddhant1 · 2019-03-22T17:01:27Z

ref #521

meiamsome

Looks great, generally, here are some comments on what you have so far.

I'd suggest keeping the PRs small so they can be dealt with quicker.

meiamsome · 2019-03-22T17:21:36Z

server/controllers/file.controller.js

+      res.json(updatedProject.files[updatedProject.files.length - 1]);
+    });
+  } catch (err) {
+    if (err) {


If statement is not required. The return after the error message can also be removed.

meiamsome · 2019-03-22T17:30:31Z

server/controllers/file.controller.js

-    .reduce((acc, childId) => (
-      [...acc, childId, ...getAllDescendantIds(files, childId)]
-    ), []);
+  return parentFile.children.reduce(


Just as a heads up, these stylistic changes should really not be included, it breaks the git blame for this line and is generally unhelpful when looking at diffs.

That said, making an additional commit to revert them would be worse in general, so only fix that if you're going to be doing it by altering commit history. I just thought I'd point out that it is something to look out for when you're creating a PR: If you have diffs on lines irrelevant to what you're doing, something usually has gone wrong :)

My global prettier generally makes these changes , since the project does not has any local .prettierrc file , my global prettier kicks in and make the changes

for this project, i squash commits for a PR, so i'm okay with you making commits to revert stuff. i also agree, these stylistic changes should not be included in this PR.

meiamsome · 2019-03-22T17:31:18Z

server/controllers/file.controller.js

+    res.json(project.files);
+  } catch (err) {
+    // NOT SO GREAT ERROR HANDLING
+    console.log(err);


We should probably respond with a 500 error here so that the API user always gets a response - though don't send the user the error

meiamsome · 2019-03-22T17:31:59Z

server/controllers/file.controller.js

      return;
    }
    res.send(resolvedFile.content);
-  });
+  } catch (err) {
+    if (err) {


Another unnecessary if here & return below

meiamsome · 2019-03-22T17:33:51Z

server/controllers/file.controller.js

+    }
+    const newFile = updatedProject.files[updatedProject.files.length - 1];
+    updatedProject.files.id(req.body.parentId).children.push(newFile.id);
+    updatedProject.save(innerErr => {


This can be promisified too.

meiamsome · 2019-03-22T17:35:08Z

server/controllers/embed.controller.js

    });
+  } catch (err) {
+    get404Sketch(html => res.send(html));
+    return;


You can delete this return

siddhant1 · 2019-03-22T18:17:30Z

Thanks for the review , I will make the changes!

siddhant1 · 2019-03-23T12:21:09Z

@meiamsome @catarak Should I make another pr when I am done refactoring other files?

catarak · 2019-03-25T21:01:46Z

@meiamsome @catarak Should I make another pr when I am done refactoring other files?
honestly, feel free to do one PR per controller. i prefer smaller PRs as they are easier to test and merge.

catarak · 2019-03-25T21:03:12Z

server/controllers/embed.controller.js

-      const filesToInject = files.filter(file => file.name.match(/\.(js|css)$/i));
-      injectMediaUrls(filesToInject, files, req.params.project_id);
+export async function serveProject(req, res) {
+  try {


i'm not a fan of the try body being huge here. it's only here to catch errors from mongodb, right? so only line 13 should be in the body.

Well, it also catches all errors that are unintended to make sure that the API user gets a response (500) when the server has a problem, instead of just having the connection hang. In non-async handlers this is handled by default by express because they try/catch all the handlers, but express doesn't support async natively. There is express-async-handler which would help us wrap these functions correctly to catch those errors, but you're just moving where the wrapping handler is.

wait, i'm confused. are you saying that express doesn't support async/await in the handler functions?

Yeah, that's right, it doesn't natively handle it, it expects the user to either throw errors synchronously, or pass them all to next() manually. Indeed if you look at the source of that async wrapper, it is essentially just doing this large try/catch there. It's a shame that express doesn't understand Promises, but the people behind express decided it'd be too dramatic a change for express to support them. (As an aside they actually decided to redesign express entirely and made Koa which is great but differs in some ways.)

So, that leaves you with two options:

Don't use async/await and lose the great syntactical bonuses that gives you, keeping in mind that you still need to handle the asynchronous errors in your promise chains by eventually passing them to next():

function (req, res, next) { // Something synchronous here promise .then(() => doSomething()) .catch(next); }

Do use async/await and be stuck with wrapping each route's handler in some sort of wrapper, be that a library one or just a big try/catch. The standard try/catch is something akin to:

async function (req, res, next) { try { // Really long function here } catch(e) { next(e); } }

wow, okay. i had no idea! thanks for the detailed explanation. my instinct is to use the express-async-handler middleware, as wrapping everything in a try/catch feels like it gets redundant fast. just to make sure i totally understand this, say the middleware were included in the project, and i wanted to catch errors specific to the mongodb query/connection. that would look like

let project; try { project = await Project.findById(req.params.project_id); } catch(e) { // do something with the error, e.g. return 404 or 500 }

@ganeshpatro321 thanks for the update! that's super frustrating. i'm starting to feel like nested callbacks is better than this mess...

The only problem with callbacks is not nesting or callbacks hell , I think if not now , we should definitely consider this as an important task in the future.

@siddhant1 i agree! i'm not a huge fan of the nested callbacks. however, it feels like the tools to move everything to async/await are in flux, and i think it's best to wait a little bit.

one option could be to just move to promises. but, honestly, and maybe this is just me, but i'm not sure the promise syntax is too much cleaner/easier to read without async/await

I think it's important to note that one of the major benefits of using Promises/(async/await) is the ability to have errors bubble up naturally, rather than not being sure whether some errors may just be being silently ignored instead of raised correctly. As discussed before, Koa is a viable Express replacement that is functionally identical except for supporting promises natively. I would be happy to do an exploration PR into swapping out for that if you think it makes sense to have a look at it @catarak ?

that is a great point.

it seems like koa is a fairly radical departure from express, and would involve some major changes. for me, there are higher priority issues, so working on this change would have to be fairly self-directed. however, i am happy to test and give support when i can. if you think there's a possibility of significantly improved error-handling and major code clean up, it's definitely worth investigating!

catarak · 2019-03-25T21:03:54Z

server/controllers/file.controller.js

-          res.json({ success: false });
-          return;
+export async function createFile(req, res) {
+  try {


same issue here. i don't like the giant try body.

Makes sense , I will refactor

catarak · 2019-03-25T21:08:29Z

server/controllers/file.controller.js

-  );
+    const newFile = updatedProject.files[updatedProject.files.length - 1];
+    updatedProject.files.id(req.body.parentId).children.push(newFile.id);
+    try {


especially nested try/catch... i would try to avoid it if possible.

Is there any way to avoid them?

@catarak @siddhant1 I have a suggestion that can clear up this try catch litter. We can define a wrapper function to catch an error and wrap every async function with it. This way we can avoid try/catch with async await.

/* Helper buddy for removing async/await try/catch litter. We can make the name of this function a little smaller */ function catchError(promise) { return promise.then(data => { if (data instanceof Error) return [data] return [null, data] }).catch(err => [err]) } // Example Usage async function usageExample(params) { const [ err, data ] = await catchError(myPromise(params)) // We are using the wrapper here if (err) { // handle error } // Do stuff with data }

What do you think about it?

hmmmm, i prefer express-async-handler as i like that it handles the error handling as middleware. to me, it feels like a cleaner solution, rather than adding a helper function to every single controller function.

Express-async-handler is good
I will refactor it along with adding a better error handling approach , because for now we are just logging the errors

i recommend not just pushing ahead on something. i think there are a few different options here and i'd like to see a comparison of these options, so we can make a decision about what the best path forward is.

catarak · 2020-02-04T22:54:39Z

closing this PR due to inactivity, but making sure this conversation is linked to in #521.

Refactor to async-await

37ebba7

Refactored

01b31da

meiamsome reviewed Mar 22, 2019

View reviewed changes

Refactor

53f988e

catarak requested changes Mar 25, 2019

View reviewed changes

catarak closed this Feb 4, 2020

catarak mentioned this pull request Feb 4, 2020

Promisify all mongoose queries using async/await #521

Open

1 task

lindapaiste mentioned this pull request Jul 23, 2023

Cleanup getProjectsForUser function #2320

Merged

4 tasks

Uh oh!

Refactor to async-await (#521) #973

Refactor to async-await (#521) #973

Uh oh!

Conversation

siddhant1 commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siddhant1 commented Mar 22, 2019

Uh oh!

siddhant1 commented Mar 22, 2019

Uh oh!

siddhant1 commented Mar 22, 2019

Uh oh!

meiamsome left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddhant1 commented Mar 22, 2019

Uh oh!

siddhant1 commented Mar 23, 2019

Uh oh!

catarak commented Mar 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meiamsome Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anku255 Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catarak commented Feb 4, 2020

siddhant1 commented Mar 22, 2019 •

edited

Loading

meiamsome Mar 26, 2019 •

edited

Loading

anku255 Apr 27, 2019 •

edited

Loading