1551829 - Upgrade to ESLint 6.1.0

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, task, P3)

Description

[Mark Banner (:standard8)]

ESLint 6 is currently in its alpha state. I've already run it, and there's some issues getting it to actually run, as well as new errors it then raises.

This is to track the work to upgrade.

Migration code:

https://eslint.org/docs/6.0.0/user-guide/migrating-to-6.0.0

Especially affecting us:

• require-atomic-updates
  • This has been added to the recommended configuration, however it seems to find too many false positives in our code - or if they are issues, we don't generally care about them, and working around them would be too difficult.
  • I filed one obvious example as https://github.com/eslint/eslint/issues/11723
  • I'm currently thinking we'll just disable this.
• There's a bug in the new configuration handling. It seems to try to handle ESLint configurations for ignored directories.
  • https://github.com/eslint/eslint/issues/11724
• no-redeclare now has builtinGlobals: true as the default option.
  • That doesn't really work for us at least in content scopes due to how we have to detect and define our globals.
  • We can have builtinGlobals on for our javascript modules though (with some fixes up to our configurations to fix newly raised issues).
• There's some sort of problem with our processors
  • filename seems to no longer be passed to the preprocess/postprocess functions and it seems an unintentional change.
  • https://github.com/eslint/eslint/issues/11725
• Unused variables are now reported for /* global entries (excellent!)
  • There's quite a few places to fix up.
• no-prototype-builtins is a newly recommended rule
  • We hit this quite a bit. Probably worth fixing.
• no-async-promise-executor also newly recommended
  • This looks quite nice and picks up unnecessary code.

Comment 1

[:Gijs (he/him)]

(In reply to Mark Banner (:standard8) from comment #0)

• no-prototype-builtins is a newly recommended rule
  • We hit this quite a bit. Probably worth fixing.

I'm an old curmudgeon from before Object.create and always thought prototype inheritance was fine and think class syntax is a bit of a gimmick (esp. with annoying differences with "normal" objects in terms of whether you want commas after methods/properties or not). So take my opinion not just with a pinch but with more like several kilos of salt.

However... creating objects without the default Object on the prototype chain seems like... a dumb thing to do? It also seems to not be done a lot outside of devtools code. Is there an alternative rule to just disallow doing that instead of verbose-ing every hasOwnProperty check ever?

(The eslint docs ( https://eslint.org/docs/rules/no-prototype-builtins ) suggest people want this in order to create Maps - but we support a "real" Map so that usecase doesn't seem worth it. The other case is people overwriting hasOwnProperty or toString, which, again, seems like a "don't do that" kind of thing.)

Comment 2

[Dave Townsend [:mossop]]

(In reply to :Gijs (he/him) from comment #1)

(In reply to Mark Banner (:standard8) from comment #0)

• no-prototype-builtins is a newly recommended rule
  • We hit this quite a bit. Probably worth fixing.

I'm an old curmudgeon from before Object.create and always thought prototype inheritance was fine and think class syntax is a bit of a gimmick (esp. with annoying differences with "normal" objects in terms of whether you want commas after methods/properties or not). So take my opinion not just with a pinch but with more like several kilos of salt.

I promise not to tread on your lawn too much.

However... creating objects without the default Object on the prototype chain seems like... a dumb thing to do? It also seems to not be done a lot outside of devtools code. Is there an alternative rule to just disallow doing that instead of verbose-ing every hasOwnProperty check ever?

(The eslint docs ( https://eslint.org/docs/rules/no-prototype-builtins ) suggest people want this in order to create Maps - but we support a "real" Map so that usecase doesn't seem worth it. The other case is people overwriting hasOwnProperty or toString, which, again, seems like a "don't do that" kind of thing.)

So looking over the rule it looks like all it does is deny using Object.prototype properties directly from a variable and instead calling them from Object.prototype ... which is quite verbose and ugly. I'd rather we not do that in general.

We are generally linting what is privileged code, in theory webpages can't manipulate us so we shouldn't need to worry about Object.prototype modification from outside. And frankly I don't think we should ever be modifying the prototypes of built-in JS objects ever or overriding their properties. I'd love it if we could have a rule that prevents all of that. I'm surprised that eslint doesn't have a rule for this already actually.

And it does look like the vast majority of hasOwnProperty calls in the code are just using {} as a map, we can switch those to real Maps or frankly I'm not sure we can't just use |"prop" in obj| within our trusted code if Maps are slower at all. That would be a straightforward thing to automatically fix with a script.

Side thought, we do have some code that runs in unprivileged pages, are there cases where web code can get into them and should we have a different eslint configuration for pages we may need to be more conservative in?

Comment 3

[Mark Banner (:standard8)]

Regarding no-prototype-builtins, I'm disabling that for now.

The patch series I'm just about to post is a WIP, though probably not far off. There will also be follow-ups to enable no-redeclare on the builtinGlobals level for JSMs, and for no-async-promise-executor on the various files I've currently disabled it on.

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1551829 - Cleanup unnecessary ESLint global definitions.

These are raised as redeclares or unused variables by ESLint 6.

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1551829 - Switch .eslintignore to not use `**` matching, as that matches only the main files not hidden ones. r?Mossop

This stops ESLint processing .eslintrc.js files in directories we're ignoring.

Depends on D37268

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1551829 - Make eslint-plugin-mozilla compatible with ESLint 6. r?Mossop

Depends on D37269

Comment 7

[Mark Banner (:standard8)]

Attachment: Bug 1551829 - Upgrade to ESLint 6.1.0. r?Mossop

This picks up various improvements, especially to how configurations are handled and some new rules.

Depends on D37270

Comment 8

[Mark Banner (:standard8)]

Attachment: Bug 1551829 - Limit exectution scope of the spidermonkey-js processor to only the directories where it is required. r?Mossop

This helps ensure that when running --fix across the entire tree, all places that can be are fixed correctly.

Depends on D37271

Comment 9

[Phabricator Automation]

Comment on attachment 9076562 [details]
Bug 1551829 - Cleanup unnecessary ESLint global definitions.

Revision D37268 was moved to bug 1571466. Setting attachment 9076562 [details] to obsolete.

Comment 10

[Mark Banner (:standard8)]

Note: I'm hoping to update soon. Currently ESLint 6.2.0 has a regression that they need to fix. I'd really like to pick up 6.2.0 or later though as it has support for BigInt, and dynamic imports (though I'm not sure if those are available in our code base).

Comment 11

[Mark Banner (:standard8)]

Unfortunately the regression with ESLint 6.2.0 looks like it is actually caused by babel-eslint. Some of the discussion on one of the PRs is indicating that it is a breaking change and may need a new major version of babel-eslint which is currently being worked towards. I'm hoping however, that another PR is enough by itself, and that'll get released soon.

In any case, having thought about it overnight, at this stage I think it is worth getting ESLint upgraded to 6.1.0, as there's some nice wins in there, and it'll mean just a small upgrade to 6.2.0 when we can.

Comment 12

[Mark Banner (:standard8)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8dc60bf37d758243c904a87e3a333446b8ca74f

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efe5e54dc20e
Switch .eslintignore to not use `**` matching, as that matches only the main files not hidden ones. r=mossop
https://hg.mozilla.org/integration/autoland/rev/3686c7bf231f
Make eslint-plugin-mozilla compatible with ESLint 6. r=mossop
https://hg.mozilla.org/integration/autoland/rev/58c0c23ac5ea
Upgrade to ESLint 6.1.0. r=mossop
https://hg.mozilla.org/integration/autoland/rev/d613bfbeb599
Limit exectution scope of the spidermonkey-js processor to only the directories where it is required. r=mossop

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/efe5e54dc20e
https://hg.mozilla.org/mozilla-central/rev/3686c7bf231f
https://hg.mozilla.org/mozilla-central/rev/58c0c23ac5ea
https://hg.mozilla.org/mozilla-central/rev/d613bfbeb599