-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
Changes from 7 commits
d123c18
96ba38e
781eda5
2fa8836
5e32a4c
ed2ea49
379c744
a3971ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -436,7 +436,10 @@ module.exports = { | |
], | ||
}, | ||
], | ||
'import/no-cycle': ['error', { ignoreExternal: true }], | ||
// TODO: Consider setting back to `ignoreExternal: true` when the expected behavior is fixed: | ||
// https://github.com/import-js/eslint-plugin-import/issues/2348#issuecomment-1587320057 | ||
LukasTy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Reevaluate when https://github.com/import-js/eslint-plugin-import/pull/2998 is merged. | ||
LukasTy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
'import/no-cycle': ['error', { ignoreExternal: false }], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: If the default is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a todo comment regarding this: 5e32a4c There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
But do we want it to be
true
? Or was it only attrue
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: