BB-672: Rewrite entity folder using async/await #980
Conversation
|
Hey @MonkeyDo , fixed the merge conflict, i guess after the Wikipedia PR, there were some changes in that particular file so there was a conflict, anyway fixed it :) |
| const editions = await Publisher.forge({bbid: res.locals.entity.bbid}) | ||
| .editions({withRelated: editionRelationsToFetch}); | ||
| res.locals.entity.editions = editions.toJSON(); | ||
| _setPublisherTitle(res); | ||
| res.locals.entity.editions.sort(entityRoutes.compareEntitiesByDate); | ||
| await entityRoutes.displayEntity(req, res); |
There was a problem hiding this comment.
This should be wrapped in a try/catch block in case one of these promises fails (for example the entity does not exist, etc.).
In the catch block, you can simply call the next function with the error (which is what we were doing before with .catch(next)).
| propsPromise.author = data && utils.entityToOption(data.toJSON()); | ||
| } | ||
| catch (err) { | ||
| log.debug(err); |
There was a problem hiding this comment.
I will suggest that we call next in all these catch blocks, like I described above, so that we catch errors a bit better.
Currently we don't get an error, but the user won't know why the entity hasn't been added to the initial relationships:
For example, with a non existing BBID: https://bookbrainz.org/work/create?author=f1906497-5614-4793-9f51-a3870f3edcad
Page loads, but no relationship (because no author entity with that BBID); I think it would be better for the user to get an error message instead.
Should be as simple as
| log.debug(err); | |
| log.debug(err); | |
| return next(err); |
| await render(props); | ||
| } | ||
| catch (err) { | ||
| log.debug(err); |
There was a problem hiding this comment.
Same here, we want tot catch the error and pass it along as we did before:
| log.debug(err); | |
| log.debug(err); | |
| next(err); |
| .catch(next); | ||
| catch (err) { | ||
| log.debug(err); | ||
| } |
There was a problem hiding this comment.
Same deal here, we need to froward the error once caught with next(err);
| if (entity.workSection) { | ||
| entity.workSection = await utils.parseLanguages(entity.workSection, req.app.locals.orm); | ||
| } | ||
| const props = await generateEntityProps( |
There was a problem hiding this comment.
I don't know why this was set up that way previously with makePromiseFromObject, but generateEntityProps does not return a promise, so there's no need to await it.
| } | ||
| ); | ||
|
|
||
|
|
There was a problem hiding this comment.
The file src/server/routes/entity/edition.ts also has the same refactor you can apply for the '/create' routes at least:
https://github.com/metabrainz/bookbrainz-site/blob/master/src/server/routes/entity/edition.ts#L278-L295
|
Hey @MonkeyDo, thanks for the review, I'm little busy for a few weeks, but I'll surely complete this PR and get it merged :) |
|
Hi @RohanSasne ! |
|
hey @MonkeyDo is this PR completed or may be i can complete this one for other files as well ? |
|
hello @Tarunmeena0901, if you have the bandwidth, you can surely take this over :) sorry @MonkeyDo , have been a lot busy lately |
|
yeah sure ...... just want to know if monkey has taken over this PR |
|
Closing in favor of #1058 |
Problem
BB-672:
Across the codebase we have asynchronous code written using promises, which is hard to read and to maintain and error-prone.
Solution
All this code was rewritten using the async/await syntax.
Areas of Impact
The changes were reflected in
src/server/routes/entity