-
Notifications
You must be signed in to change notification settings - Fork 3
Update eslint configuration to use a flat config configuration #290
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
Update eslint configuration to use a flat config configuration #290
Conversation
✅ Deploy Preview for firefox-devtools-react-contextmenu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| function configsForFiles({ files, configs }) { | ||
| return configs.map(config => ({ ...config, files })); | ||
| } |
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 could be useful to reuse in other eslint config files as well. I believe it makes it more readable by grouping all the configurations that apply to the same set of files:
...configsForFiles({
files: ['**/tests/*.js'],
configs: [
jest.configs['flat/recommended'],
jestDom.configs['flat/recommended'],
testingLibrary.configs['flat/react'],
{
rules: {
// ... other rules
}
}
]
}),The alternatives without using such a utility are much more verbose IMO:
{
files: ['**/tests/*.js'],
plugins: {
jest: jest,
"jest-dom": jestDom,
"testing-library": testingLibrary,
},
languageOptions: {
globals: jest.environments.globals.globals
},
rules: {
...jest.configs['flat/recommended'].rules,
...jestDom.configs['flat/recommended'].rules,
...testingLibrary.configs['flat/react'].rules,
'prefer-arrow-callback': 'off',
... // other rules
}
}or
{
...jest.configs['flat/recommended'],
files: ['**/tests/*.js'],
},
{
...jestDom.configs['flat/recommended'],
files: ['**/tests/*.js'],
},
{
...testingLibrary.configs['flat/react'],
files: ['**/tests/*.js'],
},
{
files: ['**/tests/*.js'],
rules: {
// other 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.
See also the last paragraph in https://eslint.org/docs/latest/use/configure/combine-configs#apply-a-config-array-to-a-subset-of-files
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.
Yeah, the helper method looks good.
I'm really not a big fan of the new config format :/ And still pissed that they had to do a huge breaking change like this...
5517a20 to
4b233f7
Compare
canova
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.
Looks good to me! I guess this project was the one that had smallest eslintrc files. Hopefully the other projects will not be too bad.
| window.resizeTo = (width, height) => { | ||
| window.innerWidth = width || window.innerWidth; | ||
| window.innerHeight = width || window.innerHeight; | ||
| window.innerHeight = height || window.innerHeight; |
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, was this warned by the eslint? That's cool.
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.
yeah, I guess this was ignored for some reason before... Not sure why :-)
| function configsForFiles({ files, configs }) { | ||
| return configs.map(config => ({ ...config, files })); | ||
| } |
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.
Yeah, the helper method looks good.
I'm really not a big fan of the new config format :/ And still pissed that they had to do a huge breaking change like this...
| } | ||
|
|
||
| // Useful to import legacy shared configuration | ||
| const compat = new FlatCompat(); |
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 did they have FlatCompat from the start? I don't remember seeing it while I was looking at it a while ago. It could make the migration a bit less painful at least.
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 not so sure :-)
I found it initially by running the migration script (that wasn't doing a super good job).
Also it's mentioned in the migration docs (https://eslint.org/docs/latest/use/configure/migration-guide#using-eslintrc-configs-in-flat-config) but not in a very visible way IMO.

This converts our existing eslint configuration to use a flat config configuration, which is needed by eslint v9.
Note that this PR doesn't update eslint, I'll let the automatic dependency updates do that later.