-
Notifications
You must be signed in to change notification settings - Fork 24
[code-infra] Bring eslint configuration from core and bump to v9 #344
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
97af236
to
de04fb8
Compare
de04fb8
to
2db8a01
Compare
2db8a01
to
1057d42
Compare
1057d42
to
2f2eea8
Compare
fdb34e6
to
b9ecdbd
Compare
7076f75
to
d420d80
Compare
c107e55
to
cd5a992
Compare
8c92e97
to
da2c6db
Compare
eslint.config.mjs
Outdated
import { includeIgnoreFile } from '@eslint/compat'; | ||
import { defineConfig, globalIgnores } from 'eslint/config'; | ||
import * as path from 'node:path'; | ||
import baseConfig from '@mui/infra/eslint'; |
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.
Would it make sense for these to be factory functions? So that we can pass configuration such as tsconfig locations:
{
name: 'Base config',
extends: createBaseConfig(),
rules: {
'import/prefer-default-export': 'off',
// No time for this
'react/prop-types': 'off',
'jsx-a11y/control-has-associated-label': 'off',
'jsx-a11y/no-autofocus': 'off',
},
},
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.
The only thing that I came across to require factory was conditionally enabling react-compiler plugin based on params.
So far in the process of migration, no other use-case has surfaced.
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 can imagine us to cook up some quite extensive factory for things like https://github.com/mui/mui-x/blob/ef29b0c82d874564c2d9a4a59d0164ffb625f9b9/.eslintrc.js#L60
🤔 Maybe it's too much future proofing, but if we make factory functions by default we don't have to make breaking changes to introduce one if needed at some point
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. Makes sense.
eslint.config.mjs
Outdated
import { defineConfig, globalIgnores } from 'eslint/config'; | ||
import * as path from 'node:path'; | ||
import baseConfig from '@mui/infra/eslint'; | ||
import testConfig from '@mui/infra/eslint-test'; |
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.
Let's keep the amount subpath exports low?
import testConfig from '@mui/infra/eslint-test'; | |
import { testConfig, baseConfig } from '@mui/infra/eslint'; |
eslint.config.mjs
Outdated
settings: { | ||
'import/resolver': { | ||
typescript: { | ||
project: ['tsconfig.node.json', 'apps/*/tsconfig.json', 'packages/*/tsconfig.json'], |
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 can probably be the default?
prettier.config.mjs
Outdated
@@ -0,0 +1,3 @@ | |||
import baseline from '@mui/infra/prettier'; |
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.
Let's use named export? Also, would it be beneficial for the future to use factory functions as well?
d65baf8
to
c8f5b38
Compare
c8f5b38
to
6c49f14
Compare
ee1a7c3
to
e909514
Compare
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.
👌 top-notch!
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.
Great work on this one! 👍 💯
Thank you for taking care of it. 🙏
Leaving some suggestions and questions.
@@ -1,6 +1,6 @@ | |||
// @ts-check | |||
const vBranchRegex = /^v\d{1,3}\.x$/; | |||
const transferLabels = ['cherry-pick']; | |||
// const transferLabels = ['cherry-pick']; |
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 like a leftover. 🙈
// const transferLabels = ['cherry-pick']; |
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/mui/mui-public.git", | ||
"directory": "packages/mui-internal-code-infra" |
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.
"directory": "packages/mui-internal-code-infra" | |
"directory": "packages/code-infra" |
}, | ||
"peerDependencies": { | ||
"eslint": "^9.0.0", | ||
"prettier": "^3.0.0" |
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 assume that typescript-eslint
should also be the peer dependency, shouldn't it? 🤔
P.S. Is there point in having only eslint
and prettier
as optional peer deps, but we still have a bunch of other direct dependencies related to eslint..? 🙈 🤷
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.
It is not needed because in the repo, you don't directly import it or try to use it in some way. It is already configured for you.
Agree about the optional peer dep. Removing it since you anyways need to install these two explicitly to access the CLI command.
@@ -0,0 +1,104 @@ | |||
import mochaPlugin from 'eslint-plugin-mocha'; |
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 not needed on mui-x
.
Maybe we could have made it opt-in as it will be eventually removed from all repos..? 🤔
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.
Making it configurable so that it can be excluded where required.
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.
or perhaps a separate mocha preset?
{ | ||
files: ['docs/**/*.md', 'docs/src/pages/**/*.{js,tsx}', 'docs/data/**/*.{js,tsx}'], | ||
options: { | ||
// otherwise code blocks overflow on the docs website | ||
// The container is 751px | ||
printWidth: 85, | ||
}, | ||
}, | ||
{ | ||
files: ['docs/pages/blog/**/*.md'], | ||
options: { | ||
// otherwise code blocks overflow on the blog website | ||
// The container is 692px | ||
printWidth: 82, | ||
}, | ||
}, |
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.
Nitpick: I'd say that these configs could/should be repo-dependent.
I.e, they might make no sense on base-ui
.
And in general, I don't understand the point in these...
Am I missing something? But the content will jump to the new line.
What's the point of limiting the length of the line? 🤷
@LukasTy Thanks for the comments. I'll address the feedback in a separate PR |
Addresses some of the comments from the previous PR #344
Addresses some of the comments from the previous PR #344
Addresses some of the comments from the previous PR #344
Signed-off-by: Lukas Tyla <[email protected]> Co-authored-by: Brijesh Bittu <[email protected]> Co-authored-by: Lukas Tyla <[email protected]>
This is now exposed as a package that can be imported and assembled with
minimum code on the client repos.
eslint-plugin-material-ui
to its own subpath.The package also exposes the common prettier config.
One of the pending discussions would be what level of abstraction do we want in this new package because a lot of the overrides in the core repo target files/folders that pertain to that repo itself and are not relevant in other repos.
Migration PR on core - mui/material-ui#46258
Base UI - mui/base-ui#2054
Closes #264