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

refactor: switch to eslint flat config #577

Merged
merged 2 commits into from
Jul 4, 2023
Merged

refactor: switch to eslint flat config #577

merged 2 commits into from
Jul 4, 2023

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 3, 2023

Switches to eslint.config.js, and newly released eslint-config-eslint v8.

Marked as refactor because this includes changes in production code.

eslint.config.js Outdated
Comment on lines 16 to 26
// eslintConfigEslint[3] is eslint-plugin-n's recommended-script config
...eslintConfigEslint.slice(0, 3),
{
files: ["**/*.js"],
...nodeRecommendedModule
},
{
files: ["**/*.cjs"],
...nodeRecommendedScript
},
...eslintConfigEslint.slice(4),
Copy link
Member Author

@mdjermanovic mdjermanovic Jul 3, 2023

Choose a reason for hiding this comment

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

This is not very convenient, perhaps eslint-config-eslint should export two configurations, one for CommonJS and one for ES Modules? Or 3 configurations: common, CommonJS-specific, and ESM-specific.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to 3 configs: the eslint-plugin-n is not really needed for web projects like eslint.org.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we can't just do ...eslintConfigESLint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason we can't just do ...eslintConfigESLint?

We can't because it's a configuration for CommonJS modules.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it.

Comment on lines -112 to +113
onToken: token => {
onToken(token) {
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 because in the new eslint-config-eslint the option avoidExplicitReturnArrows: true is set.

Comment on lines -194 to +195
function translateTemplateTokens() {
tokens.push(convertTemplatePart(that._tokens, that._code));
that._tokens = [];
}
const translateTemplateTokens = () => {
tokens.push(convertTemplatePart(this._tokens, this._code));
this._tokens = [];
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Small refactor to avoid disabling no-underscore-dangle, and I think this is a common way to avoid that = this since ES6.

Comment on lines 1 to 10
/**
* @fileoverview Generate `<test-name>.result.js` from `<test-name>.src.js` in
* `tests/fixtures/ecma-version/<number>/**` directory.
* @author Nicholas C. Zakas
* @author Toru Nagashima
*
* Usage:
* node tools/update-ecma-version-tests.js <number>
*
* @author Nicholas C. Zakas
* @author Toru Nagashima
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

Some eslint-plugin-jsdoc rules were reporting errors in the structure, so I moved the usage description under the file overview.

filename.indexOf(".src.js") > -1).map(filename =>
filename.includes(".src.js")).map(filename =>
Copy link
Member Author

Choose a reason for hiding this comment

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

unicorn/prefer-includes

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just left a couple notes.

eslint.config.js Outdated
@@ -0,0 +1,54 @@
import eslintConfigEslint from "eslint-config-eslint";
Copy link
Member

Choose a reason for hiding this comment

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

Consistent ESLint capitalization

Suggested change
import eslintConfigEslint from "eslint-config-eslint";
import eslintConfigESLint from "eslint-config-eslint";

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in ef5bdf2

eslint.config.js Outdated
Comment on lines 16 to 26
// eslintConfigEslint[3] is eslint-plugin-n's recommended-script config
...eslintConfigEslint.slice(0, 3),
{
files: ["**/*.js"],
...nodeRecommendedModule
},
{
files: ["**/*.cjs"],
...nodeRecommendedScript
},
...eslintConfigEslint.slice(4),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we can't just do ...eslintConfigESLint?

Comment on lines +29 to +34
settings: {
jsdoc: {
mode: "typescript"
}
}
},
Copy link
Member

@aladdin-add aladdin-add Jul 4, 2023

Choose a reason for hiding this comment

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

I've made a PR to move it to eslint-config-eslint.

eslint/eslint#17338

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

We'll remove it from here when we release eslint-config-eslint v9.

@nzakas
Copy link
Member

nzakas commented Jul 4, 2023

Re-approved latest changes. This is ready for merge but would like @mdjermanovic to do that to make sure I didn't miss things from other conversations.

@mdjermanovic
Copy link
Member Author

It's good now with eslint-config-eslint v8. I'll merge this now and update the config when we release eslint-config-eslint v9 with changes from eslint/eslint#17338 and eslint/eslint#17336.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants