Enable ESLint for widget/

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: standard8, Assigned: championshuttler, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

5 months ago

As part of rolling out ESLint across the tree, we should enable it for the widget/ directory.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Please note: We want to end up with two separate commits. One with the automatic changes, the second with the manual changes and the .eslintignore change. It helps with reviewing if these appear as a commit series in Phabricator.

Here's some approximate steps:

  • In .eslintignore, remove the widget/headless/tests/** and widget/tests/** lines.
  • Run eslint like this to fix most/all the issues:
    • ./mach eslint --fix widget
  • Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok
    • (especially where function arguments are spread across different lines)
  • Create a commit of the work so far, e.g.
    • hg commit -m "Bug nnn - Enable ESLint for widget (automatic changes)" widget
  • For the remaining issues, you'll need to fix them by hand. Most of those should be reasonably easy to understand, but there's more information on some specific bits (especially no-undef) here:
  • Create a second commit of the manual changes, e.g.
    • hg commit -m "Bug nnn - Enable ESLint for widget (manual changes)"
  • Post the two commits via phabricator: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
    When you post, please request initial review from myself (standard8), I'll then redirect to the owners of the code if they look good.

If you need help, ask here or on the #introduction channel on IRC: https://wiki.mozilla.org/IRC

Reporter

Updated

5 months ago
Blocks: 1357557
Reporter

Updated

5 months ago
Keywords: good-first-bug

@standard8, I would like to work on it, I build the gecko on my laptop and working on it :)

Flags: needinfo?(standard8)
Reporter

Comment 2

5 months ago

Sure, please do.

Assignee: nobody → shivams2799
Flags: needinfo?(standard8)

Hi Mark, thanks for the assigning the issue , I fixed most of the issues , I have a doubt in

/widget/tests/test_imestate.html for no-shadow (eslint) error in this snippet. What should I rename that aTest in internal function?

https://pastebin.com/8MBMrtTA

Also for the errors like

1277:5 error Use Services.obs rather than getService(). mozilla/use-services (eslint)

We need to import the +ChromeUtils.import("resource://gre/modules/Services.jsm"); right?

Thanks

Flags: needinfo?(standard8)
Reporter

Comment 4

5 months ago

(In reply to Shivam Singhal [ :championshuttler ] from comment #3)

/widget/tests/test_imestate.html for no-shadow (eslint) error in this snippet. What should I rename that aTest in internal function?

I think I'd be tempted to change those aTestInner. This maintains the a-arg style, but also makes it a different name. TBH I'm not sure the test actually needs passing into those functions, but lets not change that at this stage.

Also for the errors like

1277:5 error Use Services.obs rather than getService(). mozilla/use-services (eslint)

We need to import the +ChromeUtils.import("resource://gre/modules/Services.jsm"); right?

Yes, that should work, although if you can use this form, that would be slightly better:

const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm", null);

Flags: needinfo?(standard8)

Bug 1521928 - Enable ESLint for widget (manual changes)
Bug 1521928 - Enable ESLint for widget (automatic changes)

Attachment #9039720 - Attachment description: Bug 1521928 - Enable ESLint for widget (automatic changes) → Bug 1521928 - Enable ESLint for widget

Depends on D18067

Attachment #9039720 - Attachment is obsolete: true
Attachment #9040067 - Attachment is obsolete: true
Attachment #9040066 - Attachment is obsolete: true

Comment 10

4 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e0638c8ef27
Enable ESLint for widget (automatic changes). r=Standard8,jmathies
https://hg.mozilla.org/integration/autoland/rev/0176f4dc4345
Enable Eslint for widget (manual changes). r=Standard8,jmathies

Comment 11

4 months ago
bugherder
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

@Standard8 looks like I need to attach one more patch due to my mistake.

https://phabricator.services.mozilla.com/rMCU0176f4dc4345f1e7620f3984a7be294757616b2b#inline-36

Here it should be addObserver it should be removeObserver.

Status: RESOLVED → REOPENED
Flags: needinfo?(standard8)
Resolution: FIXED → ---
Reporter

Comment 14

4 months ago

Since we haven't had a merge happen in-between landing, I think we can just fix it here as you've already done. Generally though we'd probably just file a new bug.

Flags: needinfo?(standard8)

Comment 15

4 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9d51144a284
Changing addObserver to removeObserver. r=Standard8

Comment 16

4 months ago
bugherder
Status: REOPENED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED

Mark, are these tests something that it would make sense to uplift to beta?

Flags: needinfo?(standard8)
Reporter

Comment 18

4 months ago

No, this is just code style/formatting fixes, there's no actual bug fixes in here.

Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.