[code-infra] Migrate to flat eslint config #18562
Conversation
brijeshb42
commented
Jun 27, 2025
- I have followed (at least) the PR section of the contributing guide.
|
Deploy preview: https://deploy-preview-18562--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 0B(0.00%) - Total Gzip Change: 0B(0.00%) Show details for 100 more bundles (22 more not shown)@mui/x-charts parsed: 0B(0.00%) gzip: 0B(0.00%) |
| eslint-plugin-prettier@5.5.0: | ||
| resolution: {integrity: sha512-8qsOYwkkGrahrgoUv76NZi23koqXOGiiEzXMrT8Q7VcYaUISR+5MorIUxfWqYXN0fN/31WbSrxCxFkVQ43wwrA==} | ||
| engines: {node: ^14.18.0 || >=16.0.0} | ||
| eslint-plugin-mocha@11.1.0: |
There was a problem hiding this comment.
Nitpick: it would have been nice to avoid the reintroduction of this unused plugin. 🙈
| import { defineConfig } from 'eslint/config'; | ||
| import eslintPluginConsistentName from 'eslint-plugin-consistent-default-export-name'; | ||
| import eslintPluginJsdoc from 'eslint-plugin-jsdoc'; | ||
| import eslintPluginMuiX from 'eslint-plugin-mui-x'; |
There was a problem hiding this comment.
Nitpick: IMHO, a name with some prefix would make it more clear that this is an internal package. 🤔
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
c43cb1d to
992d5b7
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Are we still waiting for a check from someone else? 🤔 |
| 'no-restricted-imports': [ | ||
| 'error', | ||
| { | ||
| paths: RESTRICTED_TOP_LEVEL_IMPORTS.map((name) => ({ |
There was a problem hiding this comment.
Is there any logic to share with buildPackageRestrictedImports?
| '**/*.d.ts', | ||
| '**/*.spec{.ts,.tsx}', | ||
| '**/*.test{.ts,.tsx}', | ||
| `packages/${root}/src/index{.ts,.tsx,.js}`, |
There was a problem hiding this comment.
why do we allow this, we still want to force relative imports here, no?
There was a problem hiding this comment.
Good spot! 👍
This is for:
Do you suggest ignoring this line (and any other cases if they pop up) instead?
eslint.config.mjs
Outdated
| ...buildPackageRestrictedImports('@mui/x-license', 'x-license'), | ||
| ...buildPackageRestrictedImports('@mui/x-telemetry', 'x-telemetry'), | ||
|
|
||
| ...addReactCompilerRule(CHARTS_PACKAGES, ENABLE_REACT_COMPILER_PLUGIN_CHARTS), |
There was a problem hiding this comment.
Nitpick, but in general, if possible, I'd avoid building rules with files key baked in. It makes it quite hard to reason about this config to not see the files it applies to in the config object. It could make sense to build the files array instead:
{
files: Object.entries({
[`packages/x-charts/src/**/*.${EXTENSION_TS}`]: ENABLE_REACT_COMPILER_PLUGIN_CHARTS,
[`packages/x-data-grid/src/**/*.${EXTENSION_TS}`]: ENABLE_REACT_COMPILER_PLUGIN_DATA_GRID,
}).reduce((acc, [path, enabled]) => (enabled ? [...acc, path] : acc), []),
rules: {
'react-compiler/react-compiler': 'error',
},
},idk, it's the same with buildPackageRestrictedImports I guess, I have a hard time wrapping my head around it.
There was a problem hiding this comment.
Great idea. 👍
But it can probably be kept for a follow-up, since this PR only refactors ESLint to FLAT config with little to no changes in the rules themselves. 🙈
There was a problem hiding this comment.
But it can probably be kept for a follow-up
Absolutely. I would consider doing similar to buildPackageRestrictedImports:
- remove
packages/${root}/src/index{.ts,.tsx,.js}, it affects only one file, better to just add a single ignore comment than to disable the whole rule for the whole file - remove
allowRootImports, it doesn't seem to do anything - either move
filesoutside of this and do{ files: ['charts'], ...buildPackageRestrictedImports('charts') }or add an option with a config to mergebuildPackageRestrictedImports('charts', { files: ['charts'] }).
Anyway, highly opinionated, but it would break my brain less 😄
There was a problem hiding this comment.
Updated one of the above to just return files[]. This was a quick fix and now it'll have a single config with all the files instead of multiple configs for each package. Merging after CI. @LukasTy you can improve the others after merge.