-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: update project to support eslint9 and flat configs #373
base: main
Are you sure you want to change the base?
Conversation
index.mjs
Outdated
|
||
export default tseslint.config( | ||
{ | ||
files: ["**/*.ts", "**/*.tsx"], |
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 what I mention in the PR. This is the replacement of the -- ext
option. Maybe we should include here *.js
and *.jsx
?
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.
We may need a separate configuration for *.js
files that doesn't include the TS plugins
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'll try on a JS project to see what happens. It might just work fine. In this repo I had to add some tweaks for the default config so it could pick them
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.
We have a separate eslint config repo for *.js
, right?
Although I guess the point is that in TS projects we may have .js
files,... So I guess we could try adding them here and if there are problems, adding a separate config as Esau mentions
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.
We have a separate eslint config repo for *.js, right?
We do, but that project is not used at all as we don't have any JS only project
I'll play with things and see if it works!
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.
We do, but that project is not used at all as we don't have any JS only project
web-schumacher-app
still relies on it for .js
!
index.mjs
Outdated
|
||
export default tseslint.config( | ||
{ | ||
files: ["**/*.ts", "**/*.tsx"], |
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.
We may need a separate configuration for *.js
files that doesn't include the TS plugins
"lint": "echo noop", | ||
"test": "jest ./test/test.spec.js" | ||
"lint": "eslint './*.{cjs,mjs}' test/test.spec.js", | ||
"test": "node --experimental-vm-modules node_modules/jest/bin/jest.js" |
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.
Jest is still the biggest blocker for ESM... 😞
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. I remember the legacy
preset being used by several repos, but I guess it is ok to remove it in this new version and force those repos to be updated to the new default preset when bumping the dependency.
index.mjs
Outdated
|
||
export default tseslint.config( | ||
{ | ||
files: ["**/*.ts", "**/*.tsx"], |
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.
We have a separate eslint config repo for *.js
, right?
Although I guess the point is that in TS projects we may have .js
files,... So I guess we could try adding them here and if there are problems, adding a separate config as Esau mentions
a1262e4
to
776e688
Compare
BREAKING CHANGE: Several plugins with configs for sorting things have been replaced with eslint-plugin-perfectionist to consolidate plugins and better support the migration to eslint 9.
BREAKING CHANGE: Minimum required node version is 20
BREAKING CHANGE: The legacy preset no longer exists, non flat configs are still supports but require changes in the consumer
use eslint to lint the eslint config
remove snapshot in favor of checking that there are no lint errors
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.
While trying to integrate this with the react plugin, I realized that I was using the files
setting wrong. The configs should use files
as little as possible, except for when it's really needed. There are plugins that work with both JS and TS and we are now limiting them to just TS.
There are plugins like jest, that might need to be split into 2: JS and TS tests, as there are particular rules that require type checkers to be present
consolidate files into a single one
Prerelease buildBuild version: |
Release notes previewBelow is a preview of the release notes if your PR gets merged. 16.0.0 (2025-02-24)⚠ BREAKING CHANGES
Features
Tests
Continuous IntegrationBuild System
Documentation
Breaking changes file
|
@@ -0,0 +1,219 @@ | |||
const eslint = require("@eslint/js"); |
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 did a big refactor of this. Starting this thread so it's easier to add feedback.
- We now have a single definition of everything. The mjs file just imports this one
- The default export is no longer a flat config, but a function. The function takes in a few parameters. This makes it easier for a consumer to tweak some settings without having to tell what plugins are in use. See above how I disabled
typescript
for eslint in this repo. This will open it up to putting in here configs we have in other repos today (like the JS config) - I broke down the config into smaller grouped functions as it makes it easier to read. This also creates small differences with the non flat config, as rules plugins like prettier, unicorn or eslint affect all the files and not just TS files. I think this is ok.
parserOptions, | ||
}, | ||
overrideConfigFile: "./recommended.cjs", | ||
}); |
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.
at least locally I was able to get decent rule snapshot like this
const calculatedConfig =
await linter.calculateConfigForFile("./test/sample.ts");
expect(calculatedConfig.rules).toMatchSnapshot();
I tested changing some @typescript-eslint rules, some sonar rules and it seemed to catch the changes. Maybe it'll work for us?
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.
You are right the snapshot just contains the rules. I don't know what I was testing when I saw odd snapshots and it also works with flat configs
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 still wonder. Should this be a test or should this be a helper to help see what rules are affecting a file when the config is in use?
I can see good things and some friction with having the rules in the snapshot:
- Is a 1000 line snapshot valuable? There would be 3 1k line snapshots. Does it provide any value on its own? Probably not. But it can be valuable as a way to tell what has changed between 2 configs
- Any dependency update may require updating the snapshot for any new rule they add / change even if the change is backwards compatible
- With flat configs, I think the snapshot will not capture all rules, since we are running it on a specific file extension and now configs can target different types of files and it will require more long snapshots
We can add it and see if it has value long term or not
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 would defer to your judgement.
mainly, will try to shed light on the problem statement: In my case, I was looking to change a 'org-wide' default and found that I couldn't parse which eslint-config the rule originated from. The snapshot came to me as an idea to at least get some visibility of the rule output of each config. If I was tracking down again, I could find the rule via searching github.
Other use case might be to get a feel for the scope of changes in major version plugin updates, to help digest the downstream effects.
Description
This PR updates the project to be compatible with eslint v9 and flat configurations.
It introduces several breaking changes:
legacy
config preset. It's only used in a few internal repos and they should be able to update easilyApart from this I made other features / changes:
Getting the grasp of the new config setup took a while but it feels a lot more flexible.
TODOs
eslintrc
configs intoeslint.config.js
configsOpen questions
--ext
option and we have to use thefiles
option to tell it what files to lint. Should we tell it to lint alsojs
andjsx
extensions? This would allow to lint this repo better or any repo where there is still JS code<tool>Plugin
as the variable name but open to anyHow has this been tested
Changes
🚀 PR created with fotingo