Closed Bug 1248360 Opened 9 years ago Closed 9 years ago

Many eslint failures in devtools with eslint 2.0.0

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: pbro, Assigned: pbro)

Details

Attachments

(1 file)

ESLint 2.0.0 was released, and I'm guessing that that's now the default version that gets downloaded from npm with you run `npm install -g eslint`. Turns out that that's exactly what `mach eslint --setup` does, so if you run that command today, you'll get the new eslint version. Because this version comes with a number of non-backward compatible changes, the devtools codebase now fails the eslint build. At least, it looks like eslint didn't get updated on our CI environment, so builds still pass there. Here are the first issues I'm seeing: error Rule 'no-empty-label' was removed and replaced by: no-labels no-empty-label warning Rule 'space-after-keywords' was removed and replaced by: keyword-spacing space-after-keywords warning Rule 'space-return-throw-case' was removed and replaced by: keyword-spacing space-return-throw-case
These 3 errors are straightforward to fix. What's more complex is that eslint 2.0.0 seems to have changed (again) the way globals are retrieved in the no-undef rule, which means that all our custom rules that store globals (from head.js files for instance) now fail.
In \testing\eslint-plugin-mozilla\lib\helpers.js we have this helper: addVarToScope. Its job is to add "fake" globals to eslint so that when rules run that check for globals, they will find these new globals. I found that added this at the end of this function may fix the issue: scope.through.forEach((v, i) => { if (v.identifier && v.identifier.name === name) { scope.through.splice(i, 1); } }); I have no idea if this is going to have side effects. I came up with this by looking at the new version of the no-undef rule in eslint 2.0.0, which does not use ast-utils.js anymore to find globals, but instead directly uses the escope library by doing scope.through.forEach (which gives a list of all identifiers in a scope that aren't defined in this particular scope). That's why removing from the global scope's `through` array occurrences of globals fixes the issues I've been seeing. But I haven't thoroughly tested this and it's one more hack on top of existing hacks in `addVarToScope` that might again break in the future. 2 other solutions could be: - do our own no-undef rule - fix our version of eslint Dave: I'm not sure what's the correct course of actions here. I'm not too comfortable making a change like this to addVarToScope.
Flags: needinfo?(dtownsend)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #2) > In \testing\eslint-plugin-mozilla\lib\helpers.js we have this helper: > addVarToScope. > Its job is to add "fake" globals to eslint so that when rules run that check > for globals, they will find these new globals. > > I found that added this at the end of this function may fix the issue: > > scope.through.forEach((v, i) => { > if (v.identifier && > v.identifier.name === name) { > scope.through.splice(i, 1); > } > }); > > I have no idea if this is going to have side effects. I came up with this by > looking at the new version of the no-undef rule in eslint 2.0.0, which does > not use ast-utils.js anymore to find globals, but instead directly uses the > escope library by doing scope.through.forEach (which gives a list of all > identifiers in a scope that aren't defined in this particular scope). > > That's why removing from the global scope's `through` array occurrences of > globals fixes the issues I've been seeing. > But I haven't thoroughly tested this and it's one more hack on top of > existing hacks in `addVarToScope` that might again break in the future. > > 2 other solutions could be: > - do our own no-undef rule > - fix our version of eslint > > Dave: I'm not sure what's the correct course of actions here. I'm not too > comfortable making a change like this to addVarToScope. For now why don't we just make mach eslint --setup install the older version of eslint.
Flags: needinfo?(dtownsend)
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Quick try build with the ES job to make sure that still passes (although I'm not sure it runs 'mach eslint --setup' anyway since it hadn't updated to 2.0.0 last week): https://treeherder.mozilla.org/#/jobs?repo=try&revision=71c1bd29ec37
Comment on attachment 8719700 [details] MozReview Request: Bug 1248360 - Set eslint version to 1.10.3 when setting it up via mach https://reviewboard.mozilla.org/r/35061/#review31669 Looks good. Apparently I hardcoded this for the treeherder task so we'll have to update that when we switch over: https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/Dockerfile#9
Attachment #8719700 - Flags: review?(dtownsend) → review+
(In reply to Dave Townsend [:mossop] from comment #6) > Looks good. Apparently I hardcoded this for the treeherder task so we'll > have to update that when we switch over: > https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/ > Dockerfile#9 Sounds good. Thanks!
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: