Closed Bug 1309796 Opened 6 years ago Closed 6 years ago

add eslint support for netmonitor

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Iteration:
52.2 - Oct 17
Tracking Status
firefox52 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

netmonitor/test/ has eslint support, but netmonitor/ does not.

Expect:

Add .eslintrc and fix related lint error in netmonitor/ repo
Flags: qe-verify?
It's an engineering only issue
Assignee: nobody → gasolin
Flags: qe-verify? → qe-verify-
Status: NEW → ASSIGNED
When I append:

`var notUsedVar;`

into netmonitor/panel.js, and run:

`./mach eslint devtools/client/netmonitor`

I am getting an error as expected:

Z  12:5  error  'notUsedVar' is defined but never used.  no-unused-vars (eslint)

✖ 1 problem (1 error, 0 warnings)


What errors in `netmonitor/` dir do you see?

Honza
As inspector/.eslintrc , we can identify some chrome privilege requires and comment them in source

https://github.com/mozilla/gecko-dev/blob/7e0678438a65e310f2392ca79b77cba6ec08f7f4/devtools/client/inspector/.eslintrc
So, this bug is about disallowing requiring chrome code, correct?

Honza
Flags: needinfo?(gasolin)
Just want netmonitor have same configurations as inspector.

It's not actually disallowing require chrome code, but only fix warnings by addressing them with disable flags.
Flags: needinfo?(gasolin)
Comment on attachment 8800568 [details]
Bug 1309796 - add eslint support for netmonitor;

https://reviewboard.mozilla.org/r/85484/#review84132

Looks good to me, and Eslint is green for me.

R+, assuming my comment is resolved.

Thanks!
Honza

::: devtools/client/netmonitor/.eslintrc:10
(Diff revision 1)
> +  "rules": {
> +    // The inspector is being migrated to HTML and cleaned of
> +    // chrome-privileged code, so this rule disallows requiring chrome
> +    // code. Some files in the inspector disable this rule still. The
> +    // goal is to enable the rule globally on all files.
> +    "mozilla/reject-some-requires": [2, "^(chrome|chrome:.*|resource:.*|devtools/server/.*|.*\\.jsm|devtools/shared/platform/(chome|content)/.*)$"],

Please update the comment, it's used for the Network panel now (not the Inspector).
Attachment #8800568 - Flags: review?(odvarko) → review+
comment updated, thanks!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2f3dd7eec5d
add eslint support for netmonitor; r=Honza
Keywords: checkin-needed
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/b2f3dd7eec5d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Iteration: --- → 52.2 - Oct 17
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.