Closed Bug 1371293 Opened 7 years ago Closed 7 years ago

Upgrade to ESLint 4

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement, P1)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Version:
3 Branch
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

User Story

eslint-plugin-react:

- Just update.

eslint-plugin-html:

- Needs: https://github.com/BenoitZugmeyer/eslint-plugin-html/issues/65
- Needs: https://github.com/BenoitZugmeyer/eslint-plugin-html/issues/66

Attachments

(7 files)

Bug 1371293 - Upgrade ESLint to version 4.8.0, configuration changes.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1371293 - Automatically clobber node_modules when upgrading from ESLint 3 to 4.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1371293 - Fix instances of missing 'use strict;' in html files as found after ESLint 4 upgrade.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1371293 - Fix remaining instances of no-multi-spaces after upgrading to ESLint 4.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1371293 - Fix instances of padded-blocks failures after ESLint 4 upgrade.
59 bytes, text/x-review-board-request
miker
: review+
Details
Bug 1371293 - Fix instances of space-before-function-paren failures after ESLint 4 upgrade.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1371293 - Fix various devtools warnings after upgrading to ESLint 4.
59 bytes, text/x-review-board-request
miker
: review+
Details 
ESLint 4 is in pre-release, I've already played around with it a little bit, but we should start investigating what we need to do to upgrade when it is released.

So far I've found:

- The indent rule is more strict (there's a backwards-compatible option available).
- Our importing globals code doesn't work due to a change of how comments are handled within code.

I've started looking at the importing globals issue, I'll post here when I have some more updates.
Installing eslint@4.0.0 also breaks a dependency from eslint-plugin-react, which accepts only ^2.0.0 or ^3.0.0

`npm WARN eslint-plugin-react@6.10.3 requires a peer of eslint@^2.0.0 || ^3.0.0 but none was installed.`
@JonathanGB, eslint-plugin-react was updated within the last week so we'll pick that up when we do this (I'm hoping to carve out some time this week to dig into this).
(In reply to Mark Banner (:standard8) from comment #2)
> @JonathanGB, eslint-plugin-react was updated within the last week so we'll
> pick that up when we do this (I'm hoping to carve out some time this week to
> dig into this).

great!
Status: NEW → ASSIGNED
I've been doing a bit of digging, and I'm changing this to a meta bug as there's several parts to do.

The react plugin upgrades fine, however the html plugin doesn't and causes some issues (linked in the user story).

Our Mozilla plugin also needs update - I now have a fix for that, so will submit it in a new bug.
User Story: (updated)
Summary: Investigate upgrading to ESLint 4 → [meta] Investigate upgrading to ESLint 4
Depends on: 1375418
So there's an npm 5 issue which is causing problems when we update the versions of various things:

https://github.com/npm/npm/issues/17384

The workaround seems to be to nuke node_modules, but I think I'd like to see the prospects of getting that fixed before pushing on this too much more.

It may be we can get eslint-plugin-mozilla compatible without updating any sub-modules, which would at least let us move forward a little.
User Story: (updated)
The npm 5 issue is resolved in 5.1.0. eslint-plugin-mozilla was also updated a while ago.

There's still the html plugin to resolve - I'm in discussions with the author of that.
User Story: (updated)
Depends on: 1379092
Blocks: 1379669
Blocks: 1337956
Priority: -- → P1
Blocks: 1369722
Any update here? It looks like intent now has enough options that we could be able to start enforcing it and auto-fix all the issues but we need a more modern eslint for that.
Flags: needinfo?(standard8)
(In reply to Dave Townsend [:mossop] from comment #7)
> Any update here? It looks like intent now has enough options that we could
> be able to start enforcing it and auto-fix all the issues but we need a more
> modern eslint for that.

I assume you mean 'indent'?

I'd been delaying looking at it further due to the 57 work. The html plugin supports eslint 4, but breaks a lot of things for us (see the links in the user story). I'm now re-offering to help the author to try and fix those, although if you've got anyone with a bit more spare time to help out, that might be useful.

The other option is that we write our own fork / custom version - we already have one for xbl. We could potentially upgrade that and then add support for automatic fixes (which ESLint now supports for preprocessor plugins).
Flags: needinfo?(standard8)
Depends on: 1407967
Depends on: 1407968
Depends on: 1411368
Blocks: 1411914
Blocks: 1411916
Here's the plan:

- eslint-plugin-html now has an alpha version available that works for us (4.0.0-alpha.1). Since it would be very useful to start using some of the ESLint 4 features asap, I'm suggesting that we upgrade to that version.

- Given the issues we've seen with upgrades to these modules in the past, I've written some additional code to clobber the node_modules directory for upgrading ESLint from pre-3 to 4. It'll be interesting to see what reports we get.

- space-before-function-paren changed the defaults in the switch from ESLint 3 to 4. It defaults to no space before parens when using async arrow functions, i.e. `async() => ...`. That to me seems a little strange (as the extended version would be `async function()`, so I've gone for enforcing a space in the asyncArrow situation. I'm open to a little discussion here.

- I've now got a set of patches to complete the work, that I'll post in a moment.

- I won't land the patches until after 58 has branched to beta, to reduce risk there and its only a few days away now.
Summary: [meta] Investigate upgrading to ESLint 4 → Upgrade to ESLint 4
Note: I'm going with 4.8.0 instead of 4.10.0 (the latest), as there's a small issue that shows up with our xml files in 4.9.0. It is reasonably easy to fix, but I'd rather get us onto the 4.x series and able to use the benefits before we fix that.
Comment on attachment 8925941 [details]
Bug 1371293 - Upgrade ESLint to version 4.8.0, configuration changes.

https://reviewboard.mozilla.org/r/197154/#review202318

I'd like to see a bug on file to update from indent-legacy to indent.
Attachment #8925941 - Flags: review?(dtownsend) → review+
Comment on attachment 8925943 [details]
Bug 1371293 - Fix instances of missing 'use strict;' in html files as found after ESLint 4 upgrade.

https://reviewboard.mozilla.org/r/197158/#review202320
Attachment #8925943 - Flags: review?(dtownsend) → review+
Comment on attachment 8925944 [details]
Bug 1371293 - Fix remaining instances of no-multi-spaces after upgrading to ESLint 4.

https://reviewboard.mozilla.org/r/197160/#review202322
Attachment #8925944 - Flags: review?(dtownsend) → review+
Comment on attachment 8925946 [details]
Bug 1371293 - Fix instances of space-before-function-paren failures after ESLint 4 upgrade.

https://reviewboard.mozilla.org/r/197164/#review202324

Yes, I prefer a space after async.
Attachment #8925946 - Flags: review?(dtownsend) → review+
Comment on attachment 8925942 [details]
Bug 1371293 - Automatically clobber node_modules when upgrading from ESLint 3 to 4.

https://reviewboard.mozilla.org/r/197156/#review202340

::: tools/lint/eslint/setup_helper.py:59
(Diff revision 2)
>  project_root = None
>  
>  
> -def eslint_setup():
> +def eslint_maybe_setup():
> +    """Setup ESLint only if it is needed."""
> +    [has_issues, needs_clobber] = eslint_module_needs_setup()

nit: no need to wrap this in a list:

    has_issues, needs_clobber = eslint...

and later on:

    return has_issues, needs_clobber
Attachment #8925942 - Flags: review?(ahalberstadt) → review+
Blocks: 1415262
Comment on attachment 8925941 [details]
Bug 1371293 - Upgrade ESLint to version 4.8.0, configuration changes.

https://reviewboard.mozilla.org/r/197154/#review202318

I filed bug 1415262.
Blocks: 1415265
Comment on attachment 8925945 [details]
Bug 1371293 - Fix instances of padded-blocks failures after ESLint 4 upgrade.

https://reviewboard.mozilla.org/r/197162/#review202438
Attachment #8925945 - Flags: review?(mratcliffe) → review+
Comment on attachment 8925947 [details]
Bug 1371293 - Fix various devtools warnings after upgrading to ESLint 4.

https://reviewboard.mozilla.org/r/197166/#review202442
Attachment #8925947 - Flags: review?(mratcliffe) → review+
eslint-plugin-html v4 was released, so I've updated the patches to use that rather than the alpha version.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63cc92dc7a49
Upgrade ESLint to version 4.8.0, configuration changes. r=mossop
https://hg.mozilla.org/integration/autoland/rev/ae55fa3ac96b
Automatically clobber node_modules when upgrading from ESLint 3 to 4. r=ahal
https://hg.mozilla.org/integration/autoland/rev/b9a2f3e6f1cf
Fix instances of missing 'use strict;' in html files as found after ESLint 4 upgrade. r=mossop
https://hg.mozilla.org/integration/autoland/rev/27416456db70
Fix remaining instances of no-multi-spaces after upgrading to ESLint 4. r=mossop
https://hg.mozilla.org/integration/autoland/rev/17c9e4be5970
Fix instances of padded-blocks failures after ESLint 4 upgrade. r=miker
https://hg.mozilla.org/integration/autoland/rev/b2b4d1d651c3
Fix instances of space-before-function-paren failures after ESLint 4 upgrade. r=mossop
https://hg.mozilla.org/integration/autoland/rev/a60e6fbce675
Fix various devtools warnings after upgrading to ESLint 4. r=miker
Did you consider use of ignoreEOLComments instead of adding disable-rule blocks as you did?
(In reply to Philipp Kewisch [:Fallen]  from comment #50)
> Did you consider use of ignoreEOLComments instead of adding disable-rule
> blocks as you did?

I didn't consider that, but generally I don't think we should set that flag.

One downside is that it allows single-line multi space comments at the end of line - but even if that is was an option, then I still think I'd prefer to have it explicitly called out that the style is different here, and we've thought about other ways of doing it but none fit.
Depends on: 1428305
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: