-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: support eslint.config.js #95
Conversation
4f8086b
to
3830d3e
Compare
09ac7f8
to
47bb4aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this feature still very much a work in progress in ESLint core?
The official docs (https://eslint.org/docs/latest/use/configure/configuration-files-new) say:
This config system is feature complete but not enabled by default. To opt-in, place an eslint.config.js file in the root of your project or set the ESLINT_USE_FLAT_CONFIG environment variable to true.
What are the implications of this change?
Noticed EvgenyOrekhov/eslint-config-hardcore#726 when I scrolled further in my notifications and now I think I have missed an announcement on the ESLint blog or something 🙈 |
Thanks for working on this! Yes, since we've finished Phase 2 (eslint/eslint#13481) it's time for plugins to start adding support for the new config system, and in Phase 3 we have this task for this particular plugin. |
It would be nice to add metadata: https://eslint.org/docs/latest/extend/plugins#metadata-in-plugins |
👍 pushed a commit: 33045c6. 😏 |
ptla :). cc @mdjermanovic |
configs/recommended.js
Outdated
|
||
module.exports = { | ||
plugins: { n: mod }, | ||
languageOptions: { globals: mod.configs.recommended.globals }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing sourceType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure if there should be recommended
config because it would be wrong for .cjs
/.mjs
files. The current eslintrc config has overrides
for them, so it works well. We could do something similar here - export an array of configs with the files
key - but that should generally be avoided in the new config system because it makes the config more difficult to manipulate with (e.g., to apply it to a specific directory only).
For commonjs projects, user's config should typically be:
// user's eslint.config.js
const nodeRecommendedScript = require("eslint-plugin-n/configs/recommended-script");
const nodeRecommendedModule = require("eslint-plugin-n/configs/recommended-module");
module.exports = [
{
files: ["**/*.js", "**/*.cjs"],
...nodeRecommendedScript
},
{
files: ["**/*.mjs"],
...nodeRecommendedModule
}
]
For "type": "module"
projects:
// user's eslint.config.js
import nodeRecommendedScript from "eslint-plugin-n/configs/recommended-script.js";
import nodeRecommendedModule from "eslint-plugin-n/configs/recommended-module.js";
export default [
{
files: ["**/*.js", "**/*.mjs"],
...nodeRecommendedModule
},
{
files: ["**/*.cjs"],
...nodeRecommendedScript
}
];
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the beginning, to minimize the difference with the eslintrc presets, I had wondered if a configuration could be provided:
module.exports = packageJson.type === "module" ? moduleConfig : scriptConfig;
but it won't work well if it's using with ".cjs" and ".mjs". /_ \
Yes, this way sounds much clearer. Will remove it and document the usage. Thanks for the thoughtful feedback! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! I left only some minor notes.
@@ -56,7 +59,7 @@ | |||
"lint": "npm-run-all \"lint:*\"", | |||
"lint:docs": "markdownlint \"**/*.md\"", | |||
"lint:eslint-docs": "npm run update:eslint-docs -- --check", | |||
"lint:js": "eslint lib scripts tests/lib .eslintrc.js", | |||
"lint:js": "eslint .", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to the previous version, this will now lint test/fixtures
too, so just to check if that's intentional?
js.configs.recommended, | ||
nodeRecommended, | ||
...compat.extends("plugin:eslint-plugin/recommended", "prettier"), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to the previous (.eslintrc.js) config, this one might be missing this rule:
Lines 20 to 22 in bea9981
rules: { | |
"eslint-plugin/require-meta-docs-description": "error", | |
}, |
} | ||
} | ||
] | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there should also be an example with configs/recommended-module
, for ESM ("type": "module"
) projects?
c644fec
to
dcf6f66
Compare
fixes #89
an example: