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)
DevTools
General
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
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
(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 | ||
Comment 4•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/35061/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35061/
Attachment #8719700 -
Flags: review?(dtownsend)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
(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!
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Comment 9•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•