Many eslint failures in devtools with eslint 2.0.0

RESOLVED FIXED in Firefox 47

Status

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 47

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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

3 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

3 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)
(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

3 years ago
Created attachment 8719700 [details]
MozReview Request: Bug 1248360 - Set eslint version to 1.10.3 when setting it up via mach

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

3 years ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 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 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

3 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

3 years ago
Keywords: checkin-needed

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0197b0616f9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.