[charts][infra] Enable import/no-cycle eslint rules#20554
Conversation
JCQuintas
requested review from
alexfauquette and
bernardobelchior
as code owners
JCQuintas
added
type: enhancement
|
Deploy preview: https://deploy-preview-20554--material-ui-x.netlify.app/ Bundle size report
|
CodSpeed Performance ReportMerging #20554 will not alter performanceComparing Summary
Footnotes |
| return reverse ? !invertStartCoordinate : invertStartCoordinate; | ||
| } | ||
|
|
||
| export function getBarDimensions(params: { |
There was a problem hiding this comment.
The main cyclic change here was that I had to move this function from packages/x-charts/src/BarChart/useBarPlotData.ts into its own file
There was a problem hiding this comment.
Can we isolate the changes in a different PR? Ideally in this PR we'd only enable the ESLint rules and adds the type imports. Otherwise it's impossible to review ~500 files
There was a problem hiding this comment.
You will still have to trust me I didn't change any file though 😆
There was a problem hiding this comment.
It's not a trust question, it's to split code modification from type modification ;)
I 100% trust you to introduce bugs :p
There was a problem hiding this comment.
I 100% trust you to introduce bugs :p
Glad I'm hold to a high standard. It makes me thank my long history of spectacular bug implementations.
There was a problem hiding this comment.
You will still have to trust me I didn't change any file though
It isn't about trust 😄 If we're only adding type to imports there's less risk of a revert being necessary. If we bundle a lot of changes and there's an issue, reverting all the type imports would create a ridiculous amount of conflicts.
Might as well separate the changes to make them less risky and so we can more easily revert the more risk parts 😄
github-actions
Bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
Bot
removed
the
PR: out-of-date
|
By the way, we PR name is outdated now, I think |
JCQuintas
changed the title
import/no-cycle and @typescript-eslint/consistent-type-imports eslint rulesimport/no-cycle eslint rules
4 participants
I was finding cryptic errors when running #20404 in development mode.
It took me a long time to figure out the issue was cyclic deps and fix them, so we might as well enable the plugin instead 😆
The
@typescript-eslint/consistent-type-importsis important becausetype importsare not counted towards cyclic deps, as they are stripped when compiling. This makes it much easier to find the correct cyclic issues, eg: it was something like 400 errors originally and about 27 after.