-
-
Notifications
You must be signed in to change notification settings - Fork 16
refactor!: rename configs #244
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
base: v4
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR refactors the plugin's configuration structure to properly support ESLint's flat config format while maintaining backward compatibility with the legacy .eslintrc format. The main improvement is that configs now include the plugin itself, eliminating the need for users to manually specify it in their configuration.
Key changes:
- Modified config structure to include the plugin object in flat configs and plugin names array in legacy configs
- Added
legacy-prefix to all configs intended for.eslintrcformat - Updated all documentation and test files to demonstrate the new simplified setup
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/configs/config.ts | Core config structure refactored to generate both flat and legacy config variants with proper plugin inclusion |
| tests/e2e/esm/eslint.config.js | Updated to use simplified flat config syntax with spread operator |
| tests/e2e/commonjs/eslint.config.js | Updated to use simplified flat config syntax with spread operator |
| tests/e2e/eslintrc/.eslintrc.json | New test file for legacy config with legacy-stylistic-warn config |
| tests/e2e/eslintrc/test.test.ts | New E2E test to verify legacy config compatibility |
| tests/e2e/eslintrc/test.html | New test HTML file for legacy config testing |
| tests/e2e/eslintrc/package.json | Package configuration for commonjs module type |
| docs/parsers/*.md | All parser documentation updated to show new config usage patterns with flat and legacy examples |
| README.md | Added explanation of legacy- prefix for old config format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
May I request a review from you @kachkaev for this PR? You seem to be quite knowledgeable in this topic. |
kachkaev
left a comment
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.
Many thanks for working on this @schoero! I left a couple of random thoughts, but all looks great to me overall! I like that you use a mix of warnings and errors by default, that's the way to go!
docs/parsers/typescript.md
Outdated
| files: ["**/*.{ts,tsx,cts,mts}"], | ||
|
|
||
| languageOptions: { | ||
| parser: eslintParserTypeScript, |
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.
According to the latest typescript-eslint docs, the parser is added slightly differently:
import tseslint from 'typescript-eslint';
...
{
languageOptions: {
parser: tseslint.parser,
...
},It is only necessary to install typescript-eslint dependency. I haven't used legacy eslintrc config so haven't checked the latest recommended settings for it. There may be a change there too.
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 updated the package. The legacy config is still using the old package: https://typescript-eslint.io/getting-started/legacy-eslint-setup.
| }, | ||
| rules: getRulesFunction() | ||
| } | ||
| }; |
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.
Might be worth checking the type of the output (I haven't cloned the repo locally so haven't done it myself). In some plugins, pluginFooBar.configs.recommended returns Config | undefined so needs ?? {} which is inconvenient. Example: user code.
Looking at your implementation, typings may be derived correctly, it's just something worth paying extra attention to.
docs/parsers/astro.md
Outdated
| "better-tailwindcss/enforce-consistent-line-wrapping": ["warn", { printWidth: 100 }] | ||
| }, | ||
| // enable all recommended rules | ||
| ...eslintPluginBetterTailwindcss.configs.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.
From my experience, most configs contain a single flat config object, so they don't need to be spread (...). typescript-eslint (and a few others) export an array which requires a few more symbols on the consumer end but is more future-proof. I am comfortable both ways, even prefer if ... was a bit more prevelant. If you think that a single flat config object will be always enough, you can consider removing ... and returning {..} instead of [{..}]. Otherwise, it's just a matter of taste.
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.
Thank you for this in-depth review!
Now that I look at the ESLint docs again, I see that they brought back the extends field in the flat config format. It is also the recommended way for plugin configs.
I think the cleanest approach would be this one:
// eslint.config.js
import eslintPluginBetterTailwindcss from "eslint-plugin-better-tailwindcss";
import eslintParserVue from "vue-eslint-parser";
import { defineConfig } from "eslint/config";
export default defineConfig({
// enable all recommended rules
extends: [
eslintPluginBetterTailwindcss.configs.recommended
],
// if needed, override rules to configure them individually
// rules: {
// "better-tailwindcss/enforce-consistent-line-wrapping": ["warn", { printWidth: 100 }]
// },
settings: {
"better-tailwindcss": {
// tailwindcss 4: the path to the entry file of the css based tailwind config (eg: `src/global.css`)
entryPoint: "src/global.css",
// tailwindcss 3: the path to the tailwind config file (eg: `tailwind.config.js`)
tailwindConfig: "tailwind.config.js"
}
},
files: ["**/*.vue"],
languageOptions: {
parser: eslintParserVue
}
});That way
- the plugin is only loaded for the desired files
- no spread syntax is needed to merge the recommended config
- rules can still be overwritten in the same object
- the plugin settings and parsers can also be configured in the same object
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.
Oh I did not know about it. Cool!
Co-authored-by: Alexander Kachkaev <alexander@kachkaev.ru>
Fixes the predefined configs to correctly expose the plugin in the config.
This simplifies the setup as it its now possible to just add a config to
eslint.config.jswithout requiring to specify the plugin explicitly:Parsers and
languageOptionsstill need to be configured explicitly.Legacy configs using the old plugin format are now exposed using the
legacy-prefix as it is recommended by ESLintThe following configs are now exposed:
recommendedrecommended-warnrecommended-errorstylisticstylistic-warnstylistic-errorcorrectnesscorrectness-warncorrectness-errorlegacy-recommendedlegacy-recommended-warnlegacy-recommended-errorlegacy-stylisticlegacy-stylistic-warnlegacy-stylistic-errorlegacy-correctnesslegacy-correctness-warnlegacy-correctness-errorfixes: #148
closes: #152