1328804 - Enable the require-await rule for eslint

Status: RESOLVED WONTFIX

(Toolkit :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

There are four errors that are caught with this rule enabled. This rule throws an error if a function is marked async but doesn't use await.

c:\fx\toolkit\components\places\tests\bookmarks\test_sync_fields.js
  264:3   error  Async method 'tagURI' has no 'await' expression.                                                             require-await (eslint)

c:\fx\browser\components\extensions\test\browser\browser_ext_popup_sendMessage.js
  24:57  error  Async arrow function has no 'await' expression.  require-await (eslint)

c:\fx\toolkit\components\extensions\test\mochitest\test_ext_storage_content.html
   47:1   error  Async function 'contentScript' has no 'await' expression.                                                    require-await (eslint)

c:\fx\toolkit\components\extensions\test\xpcshell\test_ext_storage.js
  149:3   error  Async function 'backgroundScript' has no 'await' expression.                                                 require-await (eslint)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1328804 - Enable the require-await rule for eslint and fix the four errors found by the rule by removing the 'async' keyword from the functions that arne't using await.

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8823953 [details]
Bug 1328804 - Enable the require-await rule for eslint and fix the four errors found by the rule by removing the 'async' keyword from the functions that arne't using await.

https://reviewboard.mozilla.org/r/102434/#review103040

I'm wondering what the utility of this rule is. I think we're probably better off trying to encourage consistent use of `async` functions in places we return promises than to discourage them when we don't happen to call `await`. Is there a particular problem you're trying to prevent with this?

::: browser/components/extensions/test/browser/browser_ext_popup_sendMessage.js:26
(Diff revision 1)
>      files: {
>        "popup.html": scriptPage("popup.js"),
>        "popup.js": async function() {
> -        browser.runtime.onMessage.addListener(async msg => {
> +        browser.runtime.onMessage.addListener(msg => {
>            if (msg == "popup-ping") {
>              return Promise.resolve("popup-pong");

I'd rather we keep the `async` and return the resolution value here.

::: toolkit/components/places/tests/bookmarks/test_sync_fields.js:264
(Diff revision 1)
>        throw new Error(`Keyword ${keyword} not set for bookmark ${guid}`);
>      }
>      PlacesUtils.bookmarks.setKeywordForBookmark(id, "");
>    }
>  
> -  async tagURI(uri, tags) {
> +  tagURI(uri, tags) {

This changes this from a function that returns a promise to a function that returns undefined, which is probably not desirable.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

So I read through https://github.com/eslint/eslint/issues/6820 which is the discussion about adding the require-await rule. It's a pretty good read if you're interested and after doing so I am in agreement with you that we shouldn't push forward on this.

I'll file a separate bug about fixing the line in browser_ext_popup_sendMessage.js to not use Promise.resolve.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(filed bug 1328995 for the follow-up)