Skip to content
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.

Sign up for GitHub

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

build: track circular dependency in the codebase #29811

Merged
merged 26 commits into from
Feb 3, 2025
+1,094 −360
Merged

build: track circular dependency in the codebase #29811

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
1106021
build: add madge configuration for TypeScript circular dependency det…
dbrans Jan 20, 2025
b727bf3
feat: add circular dependency detection script
dbrans Jan 20, 2025
aac45a3
chore: add baseline circular dependencies
dbrans Jan 20, 2025
7d6639e
ci: integrate circular dependency checks into CI pipeline
dbrans Jan 20, 2025
6c2d3ba
Merge branch 'main' into djb/circular-deps-monitoring
dbrans Jan 20, 2025
15d01ca
fix lint
dbrans Jan 20, 2025
e9fc06d
add file header comment to circular-deps.json and rename to .jsonc
dbrans Jan 20, 2025
171032a
strip comments from jsonc
dbrans Jan 20, 2025
0c20e95
move circular-deps.jsonc under the development folder
dbrans Jan 28, 2025
d2992e0
rename "fix" to "update" and move RESOLUTION_STEPS to top of file
dbrans Jan 28, 2025
6a76a71
move test-circular-deps check into its own github workflow
dbrans Jan 28, 2025
ecec156
update .madgerc
dbrans Jan 28, 2025
2c594bf
Update madge dependency to latest version
dbrans Jan 28, 2025
89c7d81
fix madge imports
dbrans Jan 28, 2025
9d4d779
yarn add -D @types/madge
dbrans Jan 28, 2025
f44e290
yarn dedupe
dbrans Jan 28, 2025
0697a9b
Update LavaMoat policies
metamaskbot Jan 28, 2025
acd822a
addressing PR comments
dbrans Jan 29, 2025
2133870
Merge remote-tracking branch 'origin/main' into djb/circular-deps-mon…
dbrans Jan 29, 2025
9220391
rebuilt yarn.lock
dbrans Jan 29, 2025
45cf0fb
update circular-deps.jsonc
dbrans Jan 29, 2025
dfa5746
add circular-deps.jsonc to security team CODEOWNERS
dbrans Jan 29, 2025
f0b3de1
add @dbrans to circular-deps.jsonc CODEOWNERS
dbrans Jan 29, 2025
94e3d78
Update LavaMoat policies
metamaskbot Jan 30, 2025
7dde616
Merge remote-tracking branch 'origin/main' into djb/circular-deps-mon…
HowardBraham Jan 31, 2025
bc1c8a2
Merge branch 'main' into djb/circular-deps-monitoring
HowardBraham Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/test-lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,6 @@ jobs:

- name: Verify locales
run: yarn verify-locales --quiet

- name: Check circular dependencies
run: yarn circular-deps:check
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a great place to put this step, because test-lint was already the slowest job in this part of the pipeline. I would put this step in a separate job that runs on a separate machine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HowardBraham I think it should be added to yarn lint and yarn lint:fix, since this really is just a lint step. Developers already run yarn lint:fix (and often yarn lint) before pushing to CI, so while it might slow the lint job in CI a small amount, it won't cause total CI failures due to developers forgetting to run a yarn lint:fix plus an additional circular-lint:fix step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do both: If we make the circular dependency fix part of yarn lint:fix for developers, but run the check as a separate CI job to avoid slowing down test-lint. This gives us convenient local fixes and efficient CI checks.

Thoughts?

Copy link
Contributor Author

@dbrans dbrans Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made these changes:

  • circular-deps:check now runs as a separate CI job
  • circular-deps:update now is part of yarn lint:fix, which does not run on CI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbrans I think you forgot to make this actually run... main.yml needs to call it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

7 changes: 7 additions & 0 deletions .madgerc
View file
Open in desktop
dbrans marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"detectiveOptions": {
"ts": {
"skipTypeImports": true
}
}
}
Loading
Loading