Upgrade ESLint to v8 #3984

willdurand · 2021-10-23T16:03:09Z

Fixes #3985
Fixes #3062
Fixes #3741
Fixes #3960

This PR upgrades eslint to v8, which will allow us to support the 2022 syntax.

TODO

update addons-scanner-utils when it supports ESLint v8
update eslint-config-amo when it supports ESLint v8
add support for ESLint v8 in https://github.com/mozilla/eslint-plugin-no-unsanitized

willdurand · 2021-10-24T10:46:32Z

Sadly, we seem blocked by the ESLint config for this project (-dev deps).I think it is because we enable the browser env in our config. Not all ESlint plugins support v8 yet.

I am wondering if we could fix our config to avoid relying on some browser (e.g. react) plugins.

willdurand · 2021-10-25T08:01:21Z

I am wondering if we could fix our config to avoid relying on some browser (e.g. react) plugins.

That has been fixed. We don't prevent the installation of some (useless) packages but they are not loaded so there won't be any error, except some npm warnings about peer deps. It is fine, we use a shared config to avoid requiring/updating too many packages in too many repos.

willdurand · 2021-10-25T08:01:50Z

test-alternate fails because eslint-plugin-no-unsanitized does not support ESLint v8.

willdurand · 2021-10-25T08:02:37Z

.eslintrc

-  "rules":{
-    "no-console": 0,
-    // Inherited from airbnb via eslint-config-amo
-    "function-paren-newline": 0


This is (and always was) part of the base config.

willdurand · 2021-10-25T08:03:03Z

.eslintrc

-    "function-paren-newline": 0
+  "rules": {
+    // This project uses `console.log()`.
+    "no-console": "off"


I find off a bit easier to understand than 0. I thought we didn't need this rule but we do so I added a comment.

👌 I also prefer the more explicit "off"

willdurand · 2021-10-25T08:03:46Z

.eslintrc

-  "extends": ["amo"],
-  "parser": "babel-eslint",
+  "extends": ["amo/base"],
+  "parser": "@babel/eslint-parser",


Ha, yeah. That babel-eslint parser is no longer maintained and it has moved to the babel monorepo. That shouldn't change anything except that we use a maintained version of the same parser.

willdurand · 2021-10-25T08:04:42Z

package.json

    "@mdn/browser-compat-data": "4.0.7",
    "addons-moz-compare": "1.2.0",
-    "addons-scanner-utils": "5.0.0",
+    "addons-scanner-utils": "6.1.0",


Needed to configure ESLint with ES 2022.

willdurand · 2021-10-25T08:05:00Z

package.json

    "babel-gettext-extractor": "4.1.3",
    "babel-jest": "27.3.1",
    "babel-loader": "8.2.3",
    "codecov": "3.8.3",
    "comment-json": "4.1.1",
    "doctoc": "2.1.0",
-    "eslint-config-amo": "4.11.0",
+    "eslint-config-amo": "5.1.0",


Needed to use amo/base instead of amo.

willdurand · 2021-10-25T08:05:33Z

tests/.eslintrc

+        "expect",
+        "sinon.assert.*",
+        "assertHasMatchingError",
+        "checkMinNodeVersion",


I thought I'd go ahead and mute some more warnings. We only have 3 warnings (due to excluded files) now.

ah, Thank you for taking care of this! ❤️

I did notice those warnings not long time ago and they were kind of bothering me but didn't had the time to look into them.

willdurand · 2021-10-25T08:06:05Z

tests/unit/parsers/test.manifestjson.js

@@ -2978,11 +2978,13 @@ describe('ManifestJSONParser', () => {
    ];

    for (const invalidUrl of testInvalidUrls) {
+      // eslint-disable-next-line jest/expect-expect


I didn't update the eslint config for these test cases because testHomepageUrl is fairly specific to this describe block.

willdurand · 2021-10-25T08:06:25Z

tests/unit/scanners/test.javascript.js

-  // See: https://github.com/tc39/proposal-class-fields
-  // eslint-disable-next-line jest/no-disabled-tests
-  it.skip('should support public class fields', async () => {
+  it('should support public class fields', async () => {


willdurand · 2021-10-25T08:08:42Z

tests/unit/test.linter.js

@@ -681,7 +681,7 @@ describe('Linter.textOutput()', () => {
      addonLinter.textOutput(uselesslyTinyTerminalWidth);
      expect(addonLinter.output.summary.errors).toEqual(1);
    } catch (e) {
-      // eslint-disable-next-line jest/no-conditional-expect, jest/no-try-expect
+      // eslint-disable-next-line jest/no-conditional-expect


Removed in https://github.com/jest-community/eslint-plugin-jest/releases/tag/v25.0.0

willdurand · 2021-10-25T08:14:14Z

test-alternate fails because eslint-plugin-no-unsanitized does not support ESLint v8.

I filed an issue for that: mozilla/eslint-plugin-no-unsanitized#185

willdurand · 2021-10-25T08:16:10Z

@rpl I know test-alternate is failing but the rest of the patch is ready for a review.

tests/unit/scanners/test.html.js

rpl · 2021-10-27T09:53:44Z

@rpl I know test-alternate is failing but the rest of the patch is ready for a review.

I looked the PR and it looks in a pretty good shape.
and I see that the test-alternate failure is due to the eslint-plugin-no-unsanitized issue.

I filed an issue for that: mozilla/eslint-plugin-no-unsanitized#185

Yesterday I give it a very quick try to that locally before going afk, eslint-plugin-no-unsanitized tests were failing while running on eslint v8, but I haven't looked closely yet to the failures.
I'll try to take a more deeper look to those asap (if Freddy doesn't get to it first).

willdurand · 2021-11-02T07:48:31Z

package.json

    "eslint-plugin-async-await": "0.0.0",
-    "eslint-plugin-promise": "5.1.1",


~~not used it seems?~~

Edit: it is used for the tests actually, and it is likely coming from addons-frontend, see: https://github.com/mozilla/addons-frontend/issues/2065. I'll revert this change.

willdurand · 2021-11-02T07:49:13Z

package.json

    "babel-gettext-extractor": "4.1.3",
    "babel-jest": "27.3.1",
    "babel-loader": "8.2.3",
    "comment-json": "4.1.1",
    "doctoc": "2.1.0",
-    "eslint-config-amo": "4.11.0",
+    "eslint-config-amo": "5.1.0",
    "eslint-plugin-async-await": "0.0.0",


I am questioning the usefulness of this plugin: I don't think it is used in most of the other projects and we do use async/await everywhere now. I would like to remove this dev dependency: WDYT?

@willdurand it looks that it does only provides two "code formatting" rules, which I would guess that prettier may also be taking care of. Also the package version is v0.0.0 and last release 5 years ago, definitely 👍 on my side on getting rid of it.

codecov · 2021-11-02T07:57:13Z

Codecov Report

Merging #3984 (1454334) into master (95d2bac) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    mozilla/addons-frontend#3984   +/-   ##
=======================================
  Coverage   98.71%   98.71%           
=======================================
  Files          52       52           
  Lines        2652     2652           
  Branches      799      799           
=======================================
  Hits         2618     2618           
  Misses         34       34

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95d2bac...1454334. Read the comment docs.

willdurand · 2021-11-02T08:03:04Z

The last commit about removing eslint-plugin-promise shows that pinning the eslint-plugin-no-unsanitized version (using a git commit) fixes the issue for this project (i.e. eslint-plugin-no-unsanitized with ESLint v8 support works as intended here).

Removing eslint-plugin-promise was "needed" because this plugin does not support ESLint v8 yet and npm fails in some cases because of the peer dependency mismatch. That's annoying because we are blocked by this dev dependency now.

Apart from that, I'd like to remove eslint-plugin-async-await, which I don't find particularly useful. Thoughts?

willdurand · 2021-11-02T08:03:42Z

re-requesting a review not for the review itself but to make sure you see the last comment.

willdurand · 2021-11-03T11:05:26Z

I switched to v4.0.0 of the eslint-plugin-no-unsanitized plugin.

willdurand · 2021-11-03T11:12:49Z

@rpl for when you have time, this is ready for a review with two caveats:

eslint-plugin-promise removed because it is currently not compatible
eslint-plugin-async-await isn't removed yet but I would like to remove it, see: Upgrade ESLint to v8 #3984 (comment)

I am not sure when eslint-plugin-promise will get proper support for ESLint v8 so I am thinking about removing it here now and adding it back in a different patch when it is compatible? That would allow us to move forward given that eslint-plugin-promise is a dev dependency (and not a lib we use in the core of the linter).

rpl · 2021-11-03T11:57:37Z

@rpl for when you have time, this is ready for a review with two caveats:
* `eslint-plugin-promise` removed because it is currently not compatible

* `eslint-plugin-async-await` isn't removed yet but I would like to remove it, see: [Upgrade ESLint to v8 mozilla/addons-frontend#3984 (comment)](https://github.com/mozilla/addons-linter/pull/3984#discussion_r740788256)
I am not sure when eslint-plugin-promise will get proper support for ESLint v8 so I am thinking about removing it here now and adding it back in a different patch when it is compatible? That would allow us to move forward given that eslint-plugin-promise is a dev dependency (and not a lib we use in the core of the linter).

100% agree on eslint-plugin-async-await (#3984 (comment)).

For eslint-plugin-promise instead, it would be nice to bring it back as soon as we can, I'm ok to remove it in the short run (to allow us to proceed with upgrading eslint to v8) along with filing a follow up issue to bring it back.

rpl

@willdurand lgtm, just one detail related to @babel/eslint-parser being listed as a dependency instead of a dev dependency. Everything else looks good to me as is.

package.json

willdurand · 2021-11-03T12:16:27Z

I moved the babel parser to the list of dev dependencies and removed the async-await eslint plugin. I also filed an issue for the eslint-plugin-promise: #4016

rpl

Thanks!

This was referenced Oct 24, 2021

fix(deps): update dependency addons-scanner-utils to v6 #3987

Closed

chore(deps): update dependency eslint-config-amo to v5 #3986

Closed

willdurand commented Oct 25, 2021

View reviewed changes

willdurand marked this pull request as ready for review October 25, 2021 08:15

willdurand requested a review from rpl October 25, 2021 08:15

willdurand mentioned this pull request Oct 25, 2021

fix(deps): update dependency eslint to v8 #3958

Closed

1 task

willdurand force-pushed the eslint8 branch from 380f12f to 5ef8cf5 Compare October 26, 2021 19:36

rpl reviewed Oct 27, 2021

View reviewed changes

tests/unit/scanners/test.html.js Outdated Show resolved Hide resolved

rpl added the state:blocked_by_upstream label Oct 27, 2021

willdurand force-pushed the eslint8 branch from 5ef8cf5 to d87fd9b Compare November 2, 2021 07:40

willdurand commented Nov 2, 2021

View reviewed changes

willdurand force-pushed the eslint8 branch from c1d25c6 to 9b4bffe Compare November 2, 2021 07:54

willdurand requested a review from rpl November 2, 2021 08:03

willdurand added 10 commits November 3, 2021 11:57

Upgrade ESLint to v8

396633f

update dependencies

10c7425

Switch to maintained parser, fix eslint-config-amo

0143624

Re-allow console.log() in eslint config

dbebd9c

fix linter error

9677e37

fix eslint config for the tests

ea1a009

disable jest/expect-expect in some cases

1518185

fix comment

77bb5dd

Use commit to install mozilla/eslint-plugin-no-unsanitized

6223aa4

Add expect() in html test case

0bc06f3

willdurand force-pushed the eslint8 branch from 9b4bffe to 16814e6 Compare November 3, 2021 11:03

willdurand added 2 commits November 3, 2021 12:06

Switch to eslint-plugin-no-unsanitized 4.0.0

8dcd68c

remove eslint-plugin-promise

cb014b7

willdurand force-pushed the eslint8 branch from 16814e6 to cb014b7 Compare November 3, 2021 11:06

willdurand removed the state:blocked_by_upstream label Nov 3, 2021

willdurand mentioned this pull request Nov 3, 2021

fix(deps): update dependency eslint-plugin-no-unsanitized to v4 - autoclosed #4015

Closed

1 task

rpl requested changes Nov 3, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

willdurand added 2 commits November 3, 2021 13:11

fix deps

424470b

remove use of async-await eslint plugin

1454334

willdurand requested a review from rpl November 3, 2021 12:18

rpl approved these changes Nov 3, 2021

View reviewed changes

willdurand merged commit 2e9ab54 into master Nov 3, 2021

willdurand deleted the eslint8 branch November 3, 2021 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade ESLint to v8 #3984

Upgrade ESLint to v8 #3984

willdurand commented Oct 23, 2021 •

edited

Loading

willdurand commented Oct 24, 2021

willdurand commented Oct 25, 2021

willdurand commented Oct 25, 2021

willdurand Oct 25, 2021

willdurand Oct 25, 2021

rpl Oct 27, 2021

willdurand Oct 25, 2021

willdurand Oct 25, 2021

willdurand Oct 25, 2021

willdurand Oct 25, 2021

rpl Oct 27, 2021

willdurand Oct 25, 2021

rpl Oct 27, 2021

willdurand Oct 25, 2021

willdurand Oct 25, 2021

willdurand commented Oct 25, 2021

willdurand commented Oct 25, 2021

rpl commented Oct 27, 2021

willdurand Nov 2, 2021 •

edited

Loading

willdurand Nov 2, 2021

rpl Nov 3, 2021

codecov bot commented Nov 2, 2021 •

edited

Loading

willdurand commented Nov 2, 2021

willdurand commented Nov 2, 2021 •

edited

Loading

willdurand commented Nov 3, 2021

willdurand commented Nov 3, 2021

rpl commented Nov 3, 2021 •

edited

Loading

rpl left a comment

willdurand commented Nov 3, 2021

rpl left a comment

		"eslint-plugin-async-await": "0.0.0",
		"eslint-plugin-promise": "5.1.1",

Upgrade ESLint to v8 #3984

Upgrade ESLint to v8 #3984

Conversation

willdurand commented Oct 23, 2021 • edited Loading

TODO

willdurand commented Oct 24, 2021

willdurand commented Oct 25, 2021

willdurand commented Oct 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willdurand commented Oct 25, 2021

willdurand commented Oct 25, 2021

rpl commented Oct 27, 2021

willdurand Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2021 • edited Loading

Codecov Report

willdurand commented Nov 2, 2021

willdurand commented Nov 2, 2021 • edited Loading

willdurand commented Nov 3, 2021

willdurand commented Nov 3, 2021

rpl commented Nov 3, 2021 • edited Loading

rpl left a comment

Choose a reason for hiding this comment

willdurand commented Nov 3, 2021

rpl left a comment

Choose a reason for hiding this comment

willdurand commented Oct 23, 2021 •

edited

Loading

willdurand Nov 2, 2021 •

edited

Loading

codecov bot commented Nov 2, 2021 •

edited

Loading

willdurand commented Nov 2, 2021 •

edited

Loading

rpl commented Nov 3, 2021 •

edited

Loading