Closed Bug 1150632 Opened 7 years ago Closed 7 years ago

Extend eslint to Loop's .jsm files

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
2

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox40 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

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.
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.
Attachment #8587551 - Flags: review?(dmose)
Attachment #8587551 - Flags: feedback?(mdeboer)
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".
Attachment #8587551 - Flags: review?(dmose) → review+
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.
Attachment #8587551 - Flags: feedback?(mdeboer) → feedback+
https://hg.mozilla.org/integration/fx-team/rev/e24fc9dc7096
Iteration: --- → 40.1 - 13 Apr
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Target Milestone: --- → mozilla40
https://hg.mozilla.org/mozilla-central/rev/e24fc9dc7096
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.