Upgrade to ESLint 6.1.0
Categories
(Developer Infrastructure :: Lint and Formatting, task, P3)
Tracking
(firefox70 fixed)
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(4 files, 1 obsolete file)
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.
no-redeclare
now hasbuiltinGlobals: 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•5 years ago
|
||
(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•5 years ago
|
||
(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 thoughtprototype
inheritance was fine and thinkclass
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 everyhasOwnProperty
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 overwritinghasOwnProperty
ortoString
, 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?
Assignee | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
These are raised as redeclares or unused variables by ESLint 6.
Assignee | ||
Comment 5•5 years ago
|
||
This stops ESLint processing .eslintrc.js files in directories we're ignoring.
Depends on D37268
Assignee | ||
Comment 6•5 years ago
|
||
Depends on D37269
Assignee | ||
Comment 7•5 years ago
|
||
This picks up various improvements, especially to how configurations are handled and some new rules.
Depends on D37270
Assignee | ||
Comment 8•5 years ago
|
||
This helps ensure that when running --fix
across the entire tree, all places that can be are fixed correctly.
Depends on D37271
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
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).
Assignee | ||
Comment 11•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
Comment 13•5 years ago
|
||
Comment 14•5 years ago
|
||
bugherder |
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•2 years ago
|
Description
•