-
Notifications
You must be signed in to change notification settings - Fork 154
fix: generate configs and rules statically #1102
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
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.
This test is failing because of the dynamic import with require. I need to keep investigating.
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.
Fixed in c2ddd3a
9a4d5ea to
a551ae2
Compare
This reverts commit a551ae2.
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 pull request migrates the project from Jest to Vitest for testing, and refactors the plugin to generate configs and rules statically instead of dynamically. The migration was necessary to get certain tests running correctly with Vitest. The plugin should work the same way for end users.
Key changes:
- Replaced Jest with Vitest, including test runner, coverage tools, and ESLint plugin
- Removed dynamic generation for configs and rules, replaced with static imports and exports
- Updated test files to use Vitest imports instead of Jest globals
Reviewed changes
Copilot reviewed 19 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.setup.mts | New setup file configuring RuleTester for Vitest compatibility |
| vitest.config.mts | New Vitest configuration with coverage thresholds |
| tsconfig.json | Added **/*.mts to includes for TypeScript compilation |
| tsconfig.build.json | Excluded vitest.* files from build |
| tools/generate-configs/utils.ts | Updated to add Linter type import and satisfies clause |
| tests/lib/utils/is-testing-library-module.test.ts | Added Vitest imports |
| tests/index.test.ts | Replaced dynamic rule counting with filesystem-based validation |
| package.json | Replaced Jest dependencies with Vitest equivalents |
| lib/utils/index.ts | Removed file-import export |
| lib/utils/file-import.ts | Deleted utility for dynamic imports |
| lib/rules/index.ts | Changed from dynamic to static rule imports |
| lib/index.ts | Changed from dynamic to static config generation |
| lib/configs/*.ts | Added Linter type import and satisfies clause |
| lib/configs/index.ts | Changed from dynamic to static config imports |
| jest.config.js | Removed Jest configuration |
| eslint.config.mjs | Replaced eslint-plugin-jest with @vitest/eslint-plugin |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "typescript": "^5.7.2", | ||
| "typescript-eslint": "^8.15.0" | ||
| "typescript-eslint": "^8.15.0", | ||
| "vitest": "^3.2.4" |
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.
Can't install vitest v4 since is not compatible with node v18.
| 'testing-library/render-result-naming-convention': 'error', | ||
| }, | ||
| }; | ||
| } satisfies Linter.LegacyConfig; |
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.
Adding this for type validation purposes.
| marko, | ||
| }; | ||
|
|
||
| export default SUPPORTED_TESTING_FRAMEWORKS.reduce( |
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.
Remove dynamic generation in favor of static generation.
| const rulesDir = __dirname; | ||
| const excludedFiles = ['index']; | ||
|
|
||
| export default readdirSync(rulesDir) |
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.
Remove dynamic generation in favor of static generation.
| >), | ||
| }; | ||
| ...legacyConfigs, | ||
| 'flat/dom': { |
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.
Remove dynamic generation in favor of static generation.
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.
Fixed in c2ddd3a
| const ruleNames = Object.keys(plugin.rules); | ||
|
|
||
| // eslint-disable-next-line jest/expect-expect | ||
| it('should export all existing rules', () => { |
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 new test will take care of making sure that all rules within libs/rules are actually re-exported, so can be consumed by users installing the plugin.
The purpose for this check is to improve the DX when working on new rules, since it's quite easy to forget re-exporting the rules to the public API of the plugin.
Here, a demo of the test catching a rule not re-exported properly:
pr-1102-demo.mp4
| } | ||
| expect( | ||
| existsSync(docPath), | ||
| `Could not find documentation file for rule "${rule}" in path "${docPath}"` |
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.
Can be tested with expect assertion + custom error message with vitest.
| }); | ||
|
|
||
| // eslint-disable-next-line jest/expect-expect | ||
| it('should have the correct amount of rules', () => { |
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 is covered by the new test.
|
🎉 This PR is included in version 7.13.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Checks
Changes
Context
Relates to #855