1150632 - Extend eslint to Loop's .jsm files

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

In fixing a bug today, it really helped to have eslint available for jsm files.

As a result, I've created a patch to get it all working.

Comment 1

[Mark Banner (:standard8)]

Attachment: Extend eslint to Loop's .jsm files.

This is based on top of bug 1150273. We enable various ecmaFeatures globally, then disable them for content/shared and standalone.

MozLoopAPI.jsm uses too new features to currently be parsed, though we could deal with that in a follow-up.

The most significant changes were not defining functions within loops.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8587551 [details] [diff] [review]
Extend eslint to Loop's .jsm files.

Review of attachment 8587551 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!  r=dmose with comments addressed as appropriate.

::: browser/components/loop/.eslintignore
@@ +1,1 @@
>  MozLoopWorker.js

You might consider turning MozLoopWorker back on, if it's easy.  In either case, comments in the file explaining why these are ignored seems worthwhile.

Similarly, xpcshell and mochitest may (or may not) prove to be easy to turn on.  

All of this could easily be spun out to a future bug, however.

::: browser/components/loop/.eslintrc
@@ +8,5 @@
> +    // These are on for this directory for .jsm and content/js files.
> +    // If adding more items here, consider turning them off for the following
> +    // files if they aren't supported by all browsers.
> +    // content/shared/.eslintrc
> +    // content/standalone/.eslintrc

This seems a little error prone, since it's not DRY.  That said, the only things that jump out at me right now is moving the files out of the top-level, and I don't think we want to block this patch on that.

@@ +50,5 @@
>      "curly": 0,
>      "dot-notation": 0,
>      "eol-last": 0,
>      "eqeqeq": 0,
> +    "global-strict": 0,

According to the docs, this form will be removed in 1.0, which, as I understand it, isn't too far off, and we'll need to select one of the modes of the "strict" rule.  Can you file a spin-off bug about that?

::: browser/components/loop/GoogleImporter.jsm
@@ +214,5 @@
>      gAuthWindow.focus();
>  
>      let code;
> +
> +    function waitForTimeout() {

maybe call this promiseTimeOut to make it more obvious when reading the calling code that a promise is being returned?

@@ +578,5 @@
>      let contacts = yield db.promise("getAll");
>      let profileId = "https://www.google.com/m8/feeds/contacts/" + encodeURIComponent(gProfileId);
>      let processed = 0;
>  
> +    function skipABeat() {

Similar name tweak here?

::: browser/components/loop/LoopStorage.jsm
@@ +348,5 @@
>        onDone(new Error("Argument error: empty list"));
>        return;
>      }
>  
> +    function handler(err) {

maybe name this handleCallback?

::: browser/components/loop/content/shared/.eslintrc
@@ +8,5 @@
> +    "generators": false,
> +    "spread": false,
> +    "restParams": false,
> +    "objectLiteralShorthandMethods": false
> +  }

Doesn't this want to turn off "forOf" also?

::: browser/components/loop/standalone/.eslintrc
@@ +7,5 @@
> +    "destructuring": false,
> +    "generators": false,
> +    "spread": false,
> +    "restParams": false,
> +    "objectLiteralShorthandMethods": false

Same questions re "forOf".

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8587551 [details] [diff] [review]
Extend eslint to Loop's .jsm files.

Review of attachment 8587551 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, as long as this doesn't break anything ;)

::: browser/components/loop/GoogleImporter.jsm
@@ +587,4 @@
>      for (let [guid, contact] of Iterator(contacts)) {
>        if (++processed % kContactsChunkSize === 0) {
>          // Skip a beat every time we processed a chunk.
> +        yield skipABeat

nit: missing semicolon.

::: browser/components/loop/MozLoopService.jsm
@@ +1514,5 @@
>        MozLoopServiceInternal.notifyStatusChanged();
>      });
>    },
>  
> +  openFxASettings: Task.async(function*() {

nit: function* () {
              ^ add space here.

Comment 4

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/e24fc9dc7096

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e24fc9dc7096

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f12bd4d1d8d