Remove async/await that has no effect

[caillou]

Description

Remove async/await that has no effect.

Performance impact

This should not change how the code runs.

Security impact

None.

Checklist

• [x] My code matches the project's code style and yarn lint:fix passes.
• [x] I've added tests for the new feature, and yarn test passes.
• [ ] I have detailed the new feature in the relevant documentation.
• [ ] I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
• [ ] If this is a breaking change I've explained why.

[benjie]

Without the await:

• errors thrown from the lookupOrganizationBySlug method will not be swallowed (you’ll get an unhandled promise rejection)
• the setSlugCheckIsValid call will occur before the data is fetched which undermines the point of that variable

In general if an await occurs inside a try block it’s probably not safe to remove, even if it’s a return await ….

[caillou]

Ok, I might be completely wrong here, but lookupOrganizationBySlug does not return a Promise, but void.

There is no point awaiting it.

[caillou]

@benjie: So I took some more time to make sure my assumptions are right.

lookupOrganizationBySlug indeed returns void and there is no point awaiting it, nor does it make sense to wrap it within a try/catch block.

It is a LazyQuery. The potential error can be found in the slugError variable.

As of now setSlugCheckIsValid is always executed before lookupOrganizationBySlug returns. No matter if you await it or not. Calling setSlugCheckIsValid sets slugLoading to true, which is why the loading indicator remains.

I have a new commit that completely removes the try/catch block. Let me know if I may open a new PR with this: https://github.com/caillou/starter/commit/de26a6f236c0dcdcd8695d07606dbf36807af392

[benjie]

Ah I had forgot @singingwolfboy upgraded us to Apollo v3; in v2 this method returned a promise.

This code needs totally rewriting to work in Apollo v3 for the reasons listed above - removing the async/await would just hide that there's a bug here. The code needs to be changed such that there's a useEffect hook that calls setSlugCheckIsValid(true) when the networkStatus of the useOrganizationBySlugLazyQuery indicates that the load is complete (successful or otherwise), and the entire try/catch/finally needs removing. It would also be worth looking through the rest of the code for issues like this that were missed during the v2->v3 migration. A PR making these changes would be welcome :+1:

[benjie]

While you're at it, turning the lint rule (I think it's @typescript-eslint/await-thenable) on that makes this kind of issue an error rather than a warning would be good so we don't hit this problem again in future migrations :sweat_smile: