Closed Bug 1359540 Opened 4 years ago Closed 4 years ago

Enable the ESLint recommended rules for services/

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: standard8, Assigned: dbugs, Mentored)

References

Details

Attachments

(1 file)

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 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 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+
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)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f06e5ac97e6
Backed out changeset e6177d883f37 for eslint failures
It looks like these were failures are due to bug 1350030 that landed overnight, so the patch will need updating.
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)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20e33f2530a1
Enable the Mozilla ESlint recommended rules for services/. r=markh
https://hg.mozilla.org/mozilla-central/rev/20e33f2530a1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.