Upgrade to ESLint 4

RESOLVED FIXED in Firefox 59

Status

P1
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks: 5 bugs)

3 Branch
mozilla59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

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 attachments)

(Assignee)

Description

2 years ago
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.`
(Assignee)

Comment 2

2 years ago
@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!
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
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
(Assignee)

Updated

2 years ago
Depends on: 1375418
(Assignee)

Comment 5

2 years ago
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)
(Assignee)

Comment 6

2 years ago
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)
(Assignee)

Updated

2 years ago
Depends on: 1379092
(Assignee)

Updated

2 years ago
Blocks: 1379669
(Assignee)

Updated

2 years ago
Blocks: 1337956
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 8

2 years ago
(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)
(Assignee)

Updated

2 years ago
Depends on: 1407967
(Assignee)

Updated

2 years ago
Depends on: 1407968
(Assignee)

Updated

a year ago
Depends on: 1411368
(Assignee)

Updated

a year ago
Blocks: 1411914
(Assignee)

Updated

a year ago
Blocks: 1411916
(Assignee)

Comment 9

a year ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
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 25

a year ago
mozreview-review
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 26

a year ago
mozreview-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 27

a year ago
mozreview-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 28

a year ago
mozreview-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 29

a year ago
mozreview-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+
(Assignee)

Updated

a year ago
Blocks: 1415262
(Assignee)

Comment 30

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Blocks: 1415265
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

a year ago
eslint-plugin-html v4 was released, so I've updated the patches to use that rather than the alpha version.

Comment 48

a year ago
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?
(Assignee)

Comment 51

a year ago
(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.
(Assignee)

Updated

a year ago
Depends on: 1428305

Updated

a year ago
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.