General small fixes for no-undef eslint issues

RESOLVED FIXED in Firefox 52

Status

()

Firefox
General
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
I've scanned through a lot of the output from:

./mach lint -l eslint browser/ --rule="{no-undef:2}"

and there's simple things we can fix. The patch I'll attach fixes most of those, and as a result cuts down the number of errors by about 478 (8271 -> 7793 on my checkout).
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8802467 [details]
Bug 1311315 - General small fixes for no-undef eslint issues in browser/.

https://reviewboard.mozilla.org/r/86868/#review86066

::: browser/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
> -module.exports = {
> +module.exports = { // eslint-disable-line no-undef

This doesn't seem to scale well.

At the top of .eslintignore we specifically opt-in to having eslint run on the .eslintrc.js files.

Is this something that we should continue doing? I don't think linting our eslint files and having to manually exclude various rules will scale well or provide much benefit. If there is an issue in the eslint file that someone introduces while working on it they will likely notice it right away since that is what they are working on and focusing on.

::: browser/components/customizableui/CustomizableWidgets.jsm:1262
(Diff revision 1)
>    if (Services.appinfo.browserTabsRemoteAutostart) {
>      CustomizableWidgets.push({
>        id: "e10s-button",
>        defaultArea: CustomizableUI.AREA_PANEL,
>        onBuild: function(aDocument) {
> +        let node = aDocument.createElementNS(kNSXUL, "toolbarbutton");

Did this even work before? Yikes!
Attachment #8802467 - Flags: review?(jaws) → review+
(Assignee)

Comment 3

a year ago
mozreview-review-reply
Comment on attachment 8802467 [details]
Bug 1311315 - General small fixes for no-undef eslint issues in browser/.

https://reviewboard.mozilla.org/r/86868/#review86066

> This doesn't seem to scale well.
> 
> At the top of .eslintignore we specifically opt-in to having eslint run on the .eslintrc.js files.
> 
> Is this something that we should continue doing? I don't think linting our eslint files and having to manually exclude various rules will scale well or provide much benefit. If there is an issue in the eslint file that someone introduces while working on it they will likely notice it right away since that is what they are working on and focusing on.

Yeah, :Gijs raised this as well. I was mainly thinking its better for editing (as the editor would pick it up as well), however, having the exclusions are going to be more verbose, so maybe its just best to leave it for now - I'll drop those changes out.

> Did this even work before? Yikes!

It wouldn't have, but it is in a block that's only enabled when E10S_TESTING_ONLY is somehow set. Hence, it isn't a user-facing bug.
Comment hidden (mozreview-request)

Comment 5

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82e347fa3582
General small fixes for no-undef eslint issues in browser/. r=jaws

Comment 6

a year ago
Landed 22 hours ago https://hg.mozilla.org/mozilla-central/rev/82e347fa3582
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.