Enable the ESLint recommended rules for services/

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: dbugs, Mentored)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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 hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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)
Attachment #8862501 - Flags: review?(markh)
Comment hidden (mozreview-request)

Comment 4

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

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

Awesome, thanks!
Attachment #8862501 - Flags: review?(markh) → review+

Comment 5

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6177d883f37
Enable the Mozilla ESlint recommended rules for services/. r=markh
backed out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=95836836&repo=autoland
Flags: needinfo?(dbugs)

Comment 7

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f06e5ac97e6
Backed out changeset e6177d883f37 for eslint failures
(Reporter)

Comment 8

2 years ago
It looks like these were failures are due to bug 1350030 that landed overnight, so the patch will need updating.
Comment hidden (mozreview-request)
(Reporter)

Comment 10

2 years ago
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.
Flags: needinfo?(dbugs)

Comment 11

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20e33f2530a1
Enable the Mozilla ESlint recommended rules for services/. r=markh

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20e33f2530a1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.