-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
[code-infra] Refactor eslint import/no-cycle
rule
#42705
Conversation
Netlify deploy previewhttps://deploy-preview-42705--material-ui.netlify.app/ Bundle size report |
How is that possible? The docs specifically say:
In case it's just types that create cyclic dependencies: As far as I know import { foo } from './cyclic-dep'
// error
import type { Foo } from './cyclic-dep'
// no error This could make a refactor simpler and avoid introducing BCs |
There is more information in the linked Slack message and the following discussion.
Yes, you are correct on that, but do you think that the BC is difficult and should be avoided? |
Not at all, I'm just mentioning as I've seen devs jump through hoops to avoid cyclic dependencies, and there really isn't much benefit to it when it's about types. If the change results in higher quality code and the refactors in dependent projects are low effort then I'm all for it. |
@@ -436,7 +436,7 @@ module.exports = { | |||
], | |||
}, | |||
], | |||
'import/no-cycle': ['error', { ignoreExternal: true }], | |||
'import/no-cycle': ['error', { ignoreExternal: false }], |
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.
nit: If the default is false
, maybe it makes sense to just remove the options altogether?
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've added a todo comment regarding this: 5e32a4c
Ideally, it should be true
and hopefully it will be fixed at some point. 🤔
WDYT?
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 think it depends on the impact of import-js/eslint-plugin-import#2998
If after this change the difference between true
and false
is marginal, it could stay on false
.
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 added a mention of this PR in the comment for more context.
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.
Looking good 👍
@@ -436,7 +436,9 @@ module.exports = { | |||
], | |||
}, | |||
], | |||
'import/no-cycle': ['error', { ignoreExternal: true }], | |||
// TODO: Consider setting back to `ignoreExternal: true` when the expected behavior is fixed: |
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.
But do we want it to be true
? Or was it only at true
because we thought back then it would improve performance?
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.
Well, as you mentioned, and they specify in the readme:
Its value is false by default, but can be set to true for reducing total project lint time, if needed.
I've also reverted the path change to no avoid targetting internal packages: ed2ea49 |
If the rule passes, and it's not adding significant overhead them I don't see why we wouldn't enable it on internal packages as well. I'm also fine to keep the main focus on improving the performance in this effort. |
There is a performance difference. It's not a huge one, but it's significant enough to keep the existing rule path. 👌 |
Signed-off-by: Lukas <llukas.tyla@gmail.com>
Signed-off-by: Lukas <llukas.tyla@gmail.com>
Signed-off-by: Lukas <llukas.tyla@gmail.com>
Discussed here: https://mui-org.slack.com/archives/C042YB5RB3N/p1718977920175249
ignoreExternal
flag as it seems to improve performancefiles
path to also target internal packages (i.e. api-docs-builder)api-docs-builder
package typing structure to avoid cyclic importsReactApi
that needs to be aliased on the user side to make them unique