Closed
Bug 1311315
Opened 4 years ago
Closed 4 years ago
General small fixes for no-undef eslint issues
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(1 file)
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•4 years 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•4 years 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) |
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•4 years ago
|
||
Landed 22 hours ago https://hg.mozilla.org/mozilla-central/rev/82e347fa3582
Status: NEW → RESOLVED
Closed: 4 years 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.
Description
•