-
Notifications
You must be signed in to change notification settings - Fork 62
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!: Switch to flat config #232
Conversation
- Switch recommended config to flat config format - Rename old recommended config to recommended-legacy - Updated documentation - Upgraded ESLint - Updated tests fixes #231
Ping @eslint/eslint-team |
@@ -17,15 +17,28 @@ Lint JS, JSX, TypeScript, and more inside Markdown. | |||
|
|||
### Installing | |||
|
|||
Install the plugin alongside ESLint v6 or greater: | |||
Install the plugin alongside ESLint v8 or greater: |
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.
Which versions of ESLint will be officially supported? Per this sentence and the CI update, it looks like only >=8. However, there are still instructions for v6 and v7 below, and package.json still has "peerDependencies": { "eslint": "^6.0.0 || ^7.0.0 || ^8.0.0" }
.
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 going to update the supported ESLint versions in another PR.
eslint: [6, 7, 8] | ||
node: [12.22.0, 14, 16, 17, 18, 19, 20, 21] | ||
eslint: [8] | ||
node: [16, 17, 18, 19, 20, 21] |
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.
this also dropped some node.js/eslint versions support. To reflect this in the generated changelog:
- update the PR title.
- put these changes to a separate PR.
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.
These changes are necessary to make CI pass. I'll officially change the Node.js supported versions in a separate PR.
@@ -29,14 +29,14 @@ jobs: | |||
strategy: | |||
matrix: | |||
os: [ubuntu-latest] | |||
eslint: [6, 7, 8] | |||
node: [12.22.0, 14, 16, 17, 18, 19, 20, 21] |
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.
If we want to drop support for Node < 16, we should also update the engines
field in package.json
.
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.
As previously mentioned, I'm going to update Node.js support in a separate PR so it comes out in the changelog.
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: 唯然 <[email protected]>
tests/lib/plugin.js
Outdated
// the plugin, so we need to make sure it's resolvable and link it | ||
// if not. | ||
|
||
// eslint-disable-next-line n/no-missing-require -- Known possible failure. |
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.
Now that the eslint config for this project enables reporting unused disable directives, this directive is reported when linting is run after npm test
, which is confusing. It might be best to allow requiring eslint-plugin-markdown from this file and remove this directive (and the same one on line 1065).
// in eslint.config.js
{
files: ["tests/lib/plugin.js"],
rules: {
"n/no-missing-require": ["error", {
allowModules: ["eslint-plugin-markdown"]
}]
}
}
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.
Actually, we can now just remove the whole try-catch (lines 73-95 and 1059-1081) as there's no need to install eslint-plugin-markdown
in node_modules
.
As of ESLint v8.1.0 (eslint/eslint@a1f7ad7), plugin implementations passed to the constructor are used when resolving extends:["plugin:plugin-name/config-name"]
as well. In this case, eslint-plugin-markdown
implementation we're already passing to the ESLint
constructor will be used to get the "plugin:markdown/recommended-legacy"
config from.
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.
Ah good catch!
examples/react/eslint.config.js
Outdated
module.exports = [ | ||
js.configs.recommended, | ||
...markdown.configs.recommended, | ||
...compat.extends("plugin:react/recommended"), |
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.
As of v7.32.0, eslint-plugin-react has an additional export for the recommended config in flat config format, so we could use that for this example:
require('eslint-plugin-react/configs/recommended') // this is a flat config object
tests/lib/plugin.js
Outdated
cwd: path.resolve(__dirname, "../fixtures/"), | ||
ignore: false, | ||
overrideConfigFile: path.resolve(__dirname, "../fixtures/", fixtureConfigName), | ||
plugins: { markdown: plugin }, |
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.
plugins: { markdown: plugin }, |
This doesn't seem necessary as the config files load the plugin.
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
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!
fixes #231