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

fix(eslint): flat config support #51

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

AMorgaut
Copy link

Fix issue #50 (ESLint flat config support)

see: https://eslint.org/docs/latest/extend/plugin-migration-flat-config

recommended === false ? "off" : recommended;
return map;
}, {});
for (let { ruleName, ruleModule } of rulesList) {
Copy link
Author

@AMorgaut AMorgaut Aug 21, 2024

Choose a reason for hiding this comment

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

Previous implementation did 2 loops

The updated code does it only once

};

plugin.configs = {
recommended: {
Copy link
Author

@AMorgaut AMorgaut Aug 21, 2024

Choose a reason for hiding this comment

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

legacy configuration is maintained as-is

no expected regressions

plugins: ["@ecocode"],
rules: recommendedRules,
},
["flat/recommended"]: {
Copy link
Author

Choose a reason for hiding this comment

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

the new flat configuration follows the plugin naming convention

Comment on lines 106 to 118
| :------------------------------------------------------------------------------------- | :--------------------------------------------------------- | :---------------------------- |
| [avoid-brightness-override](docs/rules/avoid-brightness-override.md) | Should avoid to override brightness | ✅ ![badge-flat/recommended][] |
| [avoid-css-animations](docs/rules/avoid-css-animations.md) | Avoid usage of CSS animations | ✅ ![badge-flat/recommended][] |
| [avoid-high-accuracy-geolocation](docs/rules/avoid-high-accuracy-geolocation.md) | Avoid using high accuracy geolocation in web applications. | ✅ ![badge-flat/recommended][] |
| [limit-db-query-results](docs/rules/limit-db-query-results.md) | Should limit the number of returns for a SQL query | ✅ ![badge-flat/recommended][] |
| [no-empty-image-src-attribute](docs/rules/no-empty-image-src-attribute.md) | Disallow usage of image with empty source attribute | ✅ ![badge-flat/recommended][] |
| [no-import-all-from-library](docs/rules/no-import-all-from-library.md) | Should not import all from library | ✅ ![badge-flat/recommended][] |
| [no-multiple-access-dom-element](docs/rules/no-multiple-access-dom-element.md) | Disallow multiple access of same DOM element. | ✅ ![badge-flat/recommended][] |
| [no-multiple-style-changes](docs/rules/no-multiple-style-changes.md) | Disallow multiple style changes at once. | ✅ ![badge-flat/recommended][] |
| [no-torch](docs/rules/no-torch.md) | Should not programmatically enable torch mode | ✅ ![badge-flat/recommended][] |
| [prefer-collections-with-pagination](docs/rules/prefer-collections-with-pagination.md) | Prefer API collections with pagination. | ✅ ![badge-flat/recommended][] |
| [prefer-shorthand-css-notations](docs/rules/prefer-shorthand-css-notations.md) | Encourage usage of shorthand CSS notations | ✅ ![badge-flat/recommended][] |
| [provide-print-css](docs/rules/provide-print-css.md) | Enforce providing a print stylesheet | ✅ ![badge-flat/recommended][] |
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the addition of ![badge-flat/recommended][]?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't edited it myself
These changes were done by calling the yarn run update:eslint-docs command
as mentioned in the CONTRIBUTING documentation
https://github.com/green-code-initiative/ecoCode-javascript/blob/main/CONTRIBUTING.md#generate-rule-documentation

Copy link
Member

@utarwyn utarwyn Oct 17, 2024

Choose a reason for hiding this comment

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

Maybe we need to edit the configuration, because the current result is pretty strange 😞

Copy link
Author

Choose a reason for hiding this comment

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

manually fixed for now

eslint-plugin/README.md Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
{
"name": "@ecocode/eslint-plugin",
"version": "1.5.0",
"version": "1.6.0",
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to edit this yourself 😉

Copy link
Author

Choose a reason for hiding this comment

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

sure I can revert it
I guess it is done by the package release workflow

const plugin = {
meta: {
name: "@ecocode/eslint-plugin",
version: "1.6.0",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of version being hard-coded here.
This makes it necessary to modify the version bump workflow to change this value dynamically.

Copy link
Author

Choose a reason for hiding this comment

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

Agree not fan neither

The ESLint configuration says:

you should add a meta key that contains at least a name key, and ideally, a version key

So version is not mandatory and looking a other plugins, it looks like they don't include it, so we could drop it.

I'll just do some check because it also says:

Without this meta information, your plugin will not be usable with the --cache and --print-config command line options.

Even if the version is not mandatory, I would like to make sure about its impact regarding the cache feature

eslint-plugin/lib/standalone.js Outdated Show resolved Hide resolved
]
```

#### With the legacy `.eslintrc`
Copy link
Member

Choose a reason for hiding this comment

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

Same wording as ESLint documentation here

Suggested change
#### With the legacy `.eslintrc`
#### With the deprecated eslintrc format

### Enable only some rules
### Enable specific rules

#### With modern `eslint.config.js`
Copy link
Member

Choose a reason for hiding this comment

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

ESLint documentation talks about "flat configurations", so I think we can keep this term to remain consistent.

Suggested change
#### With modern `eslint.config.js`
#### With modern flat configuration

Copy link
Author

Choose a reason for hiding this comment

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

I'm unsure on that one

I think you might be right for new projects from scratch, but for existing projects and projects generated by framework generators (Nx, Angular, preact, Vue, ...) that still generate project with previous eslint configurations (which is what happened to ecocode-dashboard), it it really not obvious, and actually, it took me some time to figure out about the new eslint "flat config". What you first see when you want to add a plugin to your project is the name of your eslint configuration file

I based this part of the documentation on what I saw on another eslint plugin and which looked cristal clear to apply.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure from which plugin I originally took it but the playwright one use both "flat config" and the file name to describe how to add it. It looks to be a good compromise

Flat config (eslint.config.js)
...

Legacy config (.eslintrc)
...

https://github.com/playwright-community/eslint-plugin-playwright/blob/main/README.md

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the feedback! I think this is a great idea, we can go for it 💯

@utarwyn utarwyn added 🚀 enhancement New feature or request 🏗️ refactoring Refactoring for best practices labels Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request 🏗️ refactoring Refactoring for best practices
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the ESLint plugin compatible with the last ESLint versions with flat configurations
2 participants