1248360 - Many eslint failures in devtools with eslint 2.0.0

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Brosset <:pbro>]

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

Comment 1

[Patrick Brosset <:pbro>]

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.

Comment 2

[Patrick Brosset <:pbro>]

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.

Comment 3

[Dave Townsend [:mossop]]

(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.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: 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/

Comment 5

[Patrick Brosset <:pbro>]

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

[Dave Townsend [:mossop]]

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

Comment 7

[Patrick Brosset <:pbro>]

(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!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e0197b0616f9

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0197b0616f9