#6778: Fail CI on dependency cycles and remove madge #6857
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6857 +/- ##
=======================================
Coverage 70.42% 70.42%
=======================================
Files 1195 1195
Lines 37140 37140
Branches 6968 6968
=======================================
Hits 26154 26154
Misses 10986 10986 ☔ View full report in Codecov by Sentry. |
.eslintrc.js
Outdated
| // Enabled for the IDE, but it's disabled in the `lint` script | ||
| "import/no-cycle": "warn", | ||
| // Enabled on CI and in the IDE, but it's disabled in the local `lint` script because it's excessively slow | ||
| "import/no-cycle": "error", |
There was a problem hiding this comment.
Should I move it to the shared config?
.eslintrc.js
Outdated
| @@ -92,8 +92,8 @@ module.exports = { | |||
| // Rules that depend on https://github.com/pixiebrix/pixiebrix-extension/issues/775 | |||
| "@typescript-eslint/restrict-template-expressions": "warn", | |||
|
|
|||
| // Enabled for the IDE, but it's disabled in the `lint` script | |||
| "import/no-cycle": "warn", | |||
| // Enabled on CI and in the IDE, but it's disabled in the local `lint` script because it's excessively slow | |||
There was a problem hiding this comment.
We can continue this way because:
- it's shown in the IDE as an error
- it fails on CI if found
- it does not slow down every
npm run lint(in some cases this rule doubles the time)
| # Fixed version to avoid random breaks. Not in package.json due to #1084 | ||
| - run: npm install --global madge@5.0.1 | ||
| - run: npm run madge:circular | ||
| name: Detect circular dependencies |
There was a problem hiding this comment.
I dropped Madge because it's now redundant
| @@ -214,6 +214,7 @@ | |||
| "@types/json-stringify-safe": "^5.0.2", | |||
| "@types/lodash": "^4.14.200", | |||
| "@types/mark.js": "^8.11.10", | |||
| "@types/marked": "^6.0.0", | |||
There was a problem hiding this comment.
This also fixes one of the issues reported by:
twschiller
changed the title
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
| @@ -91,9 +91,6 @@ module.exports = { | |||
|
|
|||
| // Rules that depend on https://github.com/pixiebrix/pixiebrix-extension/issues/775 | |||
| "@typescript-eslint/restrict-template-expressions": "warn", | |||
|
|
|||
| // Enabled for the IDE, but it's disabled in the `lint` script | |||
| "import/no-cycle": "warn", | |||
There was a problem hiding this comment.
- ✅ Moved to shared config as suggested.
⚠️ The app has 8 warnings that would need to be fixed in the next update- or they can be temporarily disabled via
eslint-disable-next-line - or
"import/no-cycle": "warn",can be added there
- or they can be temporarily disabled via
| for (const ruleName of importRules) { | ||
| module.exports.rules[`import/${ruleName}`] = "off"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Restore from pixiebrix/eslint-config-pixiebrix#122
| nr lint:fast | 115s | |
| nr lint | 212s | (+84%) 😰 |
| nr lint:full | 408s | (+355%) 😱 |
lint:fast is useful when addressing issues across the codebase that are not related to import
Side note, still applies:
What does this PR do?
Reviewer Tips
eslint-nibble --rule import/no-cyclelasts longer than the full lintChecklist