fix BB-672: rewriting promises using async await in .routes/entity folder #1058
Conversation
There was a problem hiding this comment.
Thanks for the work !
There's a few tricky cases in there, and by god I know they are hard to read ad reason through!
I left a bunch of comments, some of which will need to be applied to all the files (variable naming, utility function, etc.)
Do let me know if something is unclear or if you have questions.
src/server/routes/entity/edition.ts
Outdated
| 'edition', req, res, {} | ||
| ); | ||
| async function fetchData(entity, key) { |
There was a problem hiding this comment.
Refactoring is good !
We do have a utility function in the ORM that does the same thing while also making sure to follow merged BBIDs:
https://github.com/metabrainz/bookbrainz-data-js/blob/master/src/func/entity.ts#L199-L212
So you should be able to use await orm.func.entity.getEntity(orm, 'Work', bbid, ['defaultAlias']); directly instead of rolling out this fetchData utility.
There is a small difference in that the fetch option require is set to true instead of false, but I think in this context it does make sense to ensure the entity we're trying to link does exist.
I think you might also have to move the try…catch block around all these if (req.query.xyz) so that error are caught (especially with require:true as stated above)
src/server/routes/entity/edition.ts
Outdated
| .then(render) | ||
| .catch(next); | ||
| try { | ||
| const props = await makePromiseFromObject(propsObject); |
There was a problem hiding this comment.
I think makePromiseFromObject is not needed anymore here, as it was used to await all the promises added to propsPromise.
Since we are now directly awaiting each value I think the utility doesn't do anything.
src/server/routes/entity/entity.tsx
Outdated
| catch { | ||
| // error caused by duplicates we do not want in database | ||
| return false; | ||
| } | ||
| } | ||
| else { | ||
| editorEntityVisitPromise = new Promise(resolve => resolve(false)); |
There was a problem hiding this comment.
| editorEntityVisitPromise = new Promise(resolve => resolve(false)); | |
| editorEntityVisitPromise = false; |
src/server/routes/entity/entity.tsx
Outdated
| } | ||
| catch { | ||
| // error caused by duplicates we do not want in database | ||
| return false; |
There was a problem hiding this comment.
I don't think we want to return here; the original code ended up setting editorEntityVisitPromise to a value, so in this case it should just be:
| return false; | |
| editorEntityVisitPromise = false; |
src/server/routes/entity/entity.tsx
Outdated
| @@ -84,67 +84,68 @@ export function displayEntity(req: PassportRequest, res: $Response) { | |||
|
|
|||
| let editorEntityVisitPromise; | |||
There was a problem hiding this comment.
We need to rename all these xyzPromise variables; since we are not assigning a promise to them but instead awaiting a promise and assigning the returned value directly, they should be
editorEntityVisitPromise -> editorEntityVisit
alertPromise -> alert
etc.
src/server/routes/entity/entity.tsx
Outdated
| ); | ||
| }); | ||
| const entityJSON = await entityEditPromise; | ||
| const achievementPromise = await processAchievement(orm, editorJSON.id, entityJSON); | ||
| return handler.sendPromiseResult( |
There was a problem hiding this comment.
Like above, we don't want to use handler.sendPromiseResult here but instead roll out our own try…catch block and return a response directly.
src/server/routes/entity/work.ts
Outdated
| try { | ||
| const data = await Author.forge({bbid: req.query.author}) | ||
| .fetch({require: false, withRelated: 'defaultAlias'}); | ||
| propsObject.author = utils.entityToOption(data.toJSON()); |
There was a problem hiding this comment.
I think this will change, with my suggestion above to use the orm function getEntity, but for what it's worth compared to the previous implementation you're missing that data && before calling data.toJSON.
This was in place because the entity fetch with required:false would return null or undefined, and calling undefined.toJSON() would throw an error.
|
Oh my, there are some rookie mistakes on my side. thank you for pointing them out |
Like I said, reading these promise chains and figure out what they do and when is a real pain in the ass ! |
|
I Renamed some variables , and I also took another look at the entity.tsx file. I found some more potential improvements in terms of rewriting the logic to remove promise chains. Although I attempted to rewrite the promises in a new syntax without altering the actual logic of the code, I still have my doubts about entity.tsx specially in -> L745 to L817. |
Problem
BB-672Solution
This pull request finishes the remaining tasks from PR-980. I began everything again to better understand the solution for myself.
Areas of Impact
src/server/routes/entity