Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade ESLint to v8 #3984

Merged
merged 14 commits into from
Nov 3, 2021
11 changes: 5 additions & 6 deletions .eslintrc
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
{
"rules":{
"no-console": 0,
// Inherited from airbnb via eslint-config-amo
"function-paren-newline": 0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"rules": {
// This project uses `console.log()`.
"no-console": "off"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 I also prefer the more explicit "off"

},
"env": {
"node": true,
"browser": true,
"es6": true,
},
"extends": ["amo"],
"parser": "babel-eslint",
"extends": ["amo/base"],
"parser": "@babel/eslint-parser",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"plugins": ["async-await"],
"settings": {
"async-await/space-after-async": 2,
Expand Down
1,839 changes: 385 additions & 1,454 deletions package-lock.json

Large diffs are not rendered by default.

11 changes: 5 additions & 6 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,10 @@
},
"homepage": "https://github.com/mozilla/addons-linter#readme",
"dependencies": {
"@babel/eslint-parser": "7.15.8",
"@mdn/browser-compat-data": "4.0.8",
"addons-moz-compare": "1.2.0",
"addons-scanner-utils": "5.0.0",
"addons-scanner-utils": "6.1.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to configure ESLint with ES 2022.

"ajv": "6.12.6",
"ajv-merge-patch": "4.1.0",
"chalk": "4.1.2",
Expand All @@ -54,8 +55,8 @@
"common-tags": "1.8.0",
"deepmerge": "4.2.2",
"dispensary": "0.62.0",
"eslint": "7.32.0",
"eslint-plugin-no-unsanitized": "3.2.0",
"eslint": "8.1.0",
"eslint-plugin-no-unsanitized": "4.0.0",
"eslint-visitor-keys": "3.0.0",
"espree": "9.0.0",
"esprima": "4.0.1",
Expand Down Expand Up @@ -87,15 +88,13 @@
"@babel/preset-env": "7.16.0",
"@babel/register": "7.16.0",
"babel-core": "7.0.0-bridge.0",
"babel-eslint": "10.1.0",
"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",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to use amo/base instead of amo.

"eslint-plugin-async-await": "0.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

"eslint-plugin-promise": "5.1.1",
Copy link
Member Author

@willdurand willdurand Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"github-markdown-css": "5.0.0",
"gunzip-maybe": "1.4.2",
"hashish": "0.0.4",
Expand Down
20 changes: 8 additions & 12 deletions tests/.eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,24 @@
"jest": true,
"jest/globals": true,
},
"extends": [
"plugin:promise/recommended",
],
"globals": {
"assert": true,
"sinon": true,
},
"plugins": [
"jest",
"promise",
],
"plugins": ["jest"],
"rules": {
"import/no-extraneous-dependencies": ["error", {
// Allow dev-dependencies in this directory.
"devDependencies": true
}],
"jest/expect-expect": ["warn", {
// Allow Jest or Sinon assertions
"assertFunctionNames": ["expect", "sinon.assert.*"]
// Register the custom matchers we use in this project.
"assertFunctionNames": [
"expect",
"sinon.assert.*",
"assertHasMatchingError",
"checkMinNodeVersion",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

],
}],
"promise/always-return": "off",
"promise/avoid-new": "off",
"promise/no-nesting": "off",
},
}
2 changes: 2 additions & 0 deletions tests/unit/parsers/test.manifestjson.js
Original file line number Diff line number Diff line change
Expand Up @@ -2978,11 +2978,13 @@ describe('ManifestJSONParser', () => {
];

for (const invalidUrl of testInvalidUrls) {
// eslint-disable-next-line jest/expect-expect
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

it(`should fail on forbidden homepage_url "${invalidUrl}"`, () => {
return testHomepageUrl(invalidUrl, false);
});
}

// eslint-disable-next-line jest/expect-expect
it('should not mark non forbidden homepage url as invalid', () => {
return testHomepageUrl('http://test.org', true);
});
Expand Down
6 changes: 4 additions & 2 deletions tests/unit/scanners/test.html.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,13 @@ describe('HTML', () => {
expect(linterMessages.length).toEqual(0);
});

it('should not blow up when handed malformed HTML', async () => {
it('should not blow up when handed malformed HTML', () => {
const html = validHTML('<div>Howdy <!-- >');
const htmlScanner = new HTMLScanner(html, 'index.html');

await htmlScanner.scan();
expect(async () => {
await htmlScanner.scan();
}).not.toThrow();
});

it('should return an already-parsed htmlDoc if exists', async () => {
Expand Down
6 changes: 2 additions & 4 deletions tests/unit/scanners/test.javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,7 @@ describe('JavaScript Scanner', () => {
expect(linterMessages).toEqual([]);
});

// 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 () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

const code = 'class MyClass { a = 1; }';

const jsScanner = new JavaScriptScanner(code, 'code.js');
Expand Down Expand Up @@ -286,7 +284,7 @@ describe('JavaScript Scanner', () => {
// don't change behaviour on us.
// https://github.com/mozilla/addons-linter/pull/98#issuecomment-158890847
it('ignores /*global foo*/', () => {
const eslint = ESLint.linter;
const eslint = new ESLint.Linter();
const config = { rules: { test: 2 } };
let ok = false;

Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test.linter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(false).toBe(true);
}
});
Expand Down