1359540 - Enable the ESLint recommended rules for services/

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Banner (:standard8)]

I'm preparing to get the general ESLint rules we have in the eslint-plugin-mozilla recommended configuration spread around the tree more widely.

At the moment, services/ doesn't inherit rules from the mozilla/recommended configuration, and so just gets a basic set.

The aim of this bug is to get services/ passing mozilla/recommended and for it to be enabled by default.

Comment 1

[Dan Banner]

Attachment: Bug 1359540 - Enable the Mozilla ESlint recommended rules for services/.

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

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8862501 [details]
Bug 1359540 - Enable the Mozilla ESlint recommended rules for services/.

https://reviewboard.mozilla.org/r/134404/#review137412

Thanks for the patch and for your work improving the general quality of Firefox, but there are a number of issues here that need to be addressed. I'm happy to take another look when you update the patch!

::: services/crypto/modules/utils.js:514
(Diff revision 1)
>        // trailing "==" padding.
>        let hash_b64 = btoa(hash);
>        artifacts.hash = hash_b64;
>      }
>  
> -    let requestString = ("hawk.1.header" + "\n" +
> +    let requestString = ("hawk.1.headerFxAccountsManager.jsm\n" +

The addition of "FxAccountsManager.jsm" here looks wrong

::: services/fxaccounts/FxAccountsManager.jsm:468
(Diff revision 1)
>      }
>  
>      let client = this._getFxAccountsClient();
>      return client.accountExists(aEmail).then(
>        result => {
> -        log.debug("Account " + result ? "" : "does not" + " exists");
> +        log.debug("Account " + result ? "" : "does not exists");

Before this change, the resulting string would be either "Account exists" or "Account does not exists" (even though that last one is bad grammar :) However, with your change the string with be either "Account " or "Account does not exists". I'm not sure why eslint is complaining about this line - it might be that you just need to add parens (ie, brackets) - ie, |"Account " + (result ? "" : "does not" + " exists")|, but I'm not sure.

::: services/sync/modules/bookmark_repair.js:384
(Diff revision 1)
>          break; // our caller will take the abort action.
>  
>        case STATE.FINISHED:
>          break;
>  
> -      case NOT_REPAIRING:
> +      case STATE.NOT_REPAIRING:

this change looks like it is fixing a real bug - thanks! :)

::: services/sync/modules/telemetry.js:142
(Diff revision 1)
>      let took = timeDeltaFrom(this.startTime);
>      if (took > 0) {
>        this.took = took;
>      }
>      if (error) {
> -      this.failureReason = SyncTelemetry.transformError(error);
> +      this.failureReason = this.SyncTelemetry.transformError(error);

This change is wrong - |this| here will be an EngineRecord, not the module scope where SyncTelemetry is defined. I pulled this code to try and see what eslint was complaining about here, but when I couldn't see any eslint errors without the additional |this|, so I'm not sure why this change is necessary (but if it is, a solution might be to change the last line of the file from:

this.SyncTelemetry = new SyncTelemetryImpl(ENGINES);

to something like:

let SyncTelemetry = this.SyncTelemetry = new SyncTelemetryImpl(ENGINES);

(but even that seems strange and I'd have thought unnecessary)

::: services/sync/tests/unit/head_helpers.js:488
(Diff revision 1)
>  Utils.getDefaultDeviceName = function() {
>    return "Test device name";
>  };
>  
>  function registerRotaryEngine() {
> +  let RotaryEngine =

This needs to read |let {RotaryEngine} =| (or maybe |let { RotaryEngine } =|, depending on what style we are heading towards - :Standard8 probably knows)

Comment 3

[Dan Banner]

Comment on attachment 8862501 [details]
Bug 1359540 - Enable the Mozilla ESlint recommended rules for services/.

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

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8862501 [details]
Bug 1359540 - Enable the Mozilla ESlint recommended rules for services/.

https://reviewboard.mozilla.org/r/134404/#review138192

Awesome, thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6177d883f37
Enable the Mozilla ESlint recommended rules for services/. r=markh

Comment 6

[Carsten Book [:Tomcat]]

backed out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=95836836&repo=autoland

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f06e5ac97e6
Backed out changeset e6177d883f37 for eslint failures

Comment 8

[Mark Banner (:standard8)]

It looks like these were failures are due to bug 1350030 that landed overnight, so the patch will need updating.

Comment 9

[Dan Banner]

Comment on attachment 8862501 [details]
Bug 1359540 - Enable the Mozilla ESlint recommended rules for services/.

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

Comment 10

[Mark Banner (:standard8)]

The additional changes due to bug 1350030 are minor, and I've checked them in the updated patch and rs=me for those extra bits. I'll get this re-landed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20e33f2530a1
Enable the Mozilla ESlint recommended rules for services/. r=markh

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20e33f2530a1