1381049 - Fix cases of async function definitions without await in Places code

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement)

Description

[Mark Banner (:standard8)]

We've got a few cases in Places related code where we're defining async functions but they don't actually use await, and therefore don't need to be defined as async.

Some of them do return promises though so the callers don't need updating.

This is part of the work towards enabling the eslint rule in bug 1381030.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1381049 - Fix cases of async function definitions without await in Places code.

Review commit: https://reviewboard.mozilla.org/r/157832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157832/

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8887097 [details]
Bug 1381049 - Fix cases of async function definitions without await in Places code.

https://reviewboard.mozilla.org/r/157832/#review162974

can we enable the rule just for Places with a local .eslintrc? Is it not worth it?

::: toolkit/components/places/PlacesDBUtils.jsm:223
(Diff revision 1)
>      await PlacesUtils.withConnectionWrapper(
>        "PlacesDBUtils: coherence check:",
> -      db => db.executeTransaction(async () => {
> +      db => db.executeTransaction(() => {
>          for (let {query, params} of stmts) {
>            params = params ? params : null;
>            allStatementsPromises.push(db.execute(query, params).catch(ex => {

hm, I'm worried this may indicate a bug, for the transaction to be useful the queries should be await-ed for, or they may slip out of the transaction.

I don't think there's an actual win here into collecting all the promises and awaiting later, since all the queries on a database connection are serialized regardless.

Can we rewrite this as a plain for loop with awaits inside it?

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8887097 [details]
Bug 1381049 - Fix cases of async function definitions without await in Places code.

https://reviewboard.mozilla.org/r/157832/#review162974

I was thinking of holding off as I'm hoping to get it enabled globally soon, however with PTO etc, I decided it was worth it, so I've added on just to toolkit/components/places for now. We'll get the rest soon.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8887097 [details]
Bug 1381049 - Fix cases of async function definitions without await in Places code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157832/diff/1-2/

Comment 5

[Mark Banner (:standard8)]

Note: the editBookmarkOverlay.js changes in the original version of the patch landed in bug 1381027.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d89b33a35a3
Fix cases of async function definitions without await in Places code. r=mak

Comment 7

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/9d89b33a35a3