ci: enable circular dep linting #30148
Open
+735
−119
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [d9391f4]
Page Load Metrics (1687 ± 73 ms)
Bundle size diffs
|
davidmurdoch
force-pushed
the
enable-circular-dep-lint
branch
from
d9391f4 to
077da6a
Compare
|
New and updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
Builds ready [8e2731e]
Page Load Metrics (1641 ± 64 ms)
Bundle size diffs
|
Builds ready [7ddc5bf]
Page Load Metrics (1558 ± 60 ms)
Bundle size diffs
|
|
@metamaskbot update-policies |
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
Builds ready [194c797]
Page Load Metrics (1744 ± 130 ms)
Bundle size diffs
|
Builds ready [a58a5c6]
Page Load Metrics (1609 ± 78 ms)
Bundle size diffs
|
Builds ready [45b07bd]
Page Load Metrics (1647 ± 91 ms)
Bundle size diffs
|
Builds ready [bbb63a1]
Page Load Metrics (2045 ± 121 ms)
Bundle size diffs
|
davidmurdoch
requested review from
HowardBraham,
dbrans and
a team
as code owners
Builds ready [1b087ed]
Page Load Metrics (1658 ± 58 ms)
Bundle size diffs
|
davidmurdoch
commented
Feb 25, 2025
| @@ -1,8 +1,18 @@ | |||
| #!/usr/bin/env tsx | |||
| #!/usr/bin/env -S node --require "./node_modules/tsx/dist/preflight.cjs" --import "./node_modules/tsx/dist/loader.mjs" | |||
There was a problem hiding this comment.
Not really necessary, but this is how tsx is used in some of our other modules (in webpack). Only reason for doing it this way is that it is slightly faster.
We don't actually need it here (or in webpack), since we aren't launching these scripts directly. I guess it's more of a "technically correct" than actually useful.
In otherwords: it is safe to ignore this change, safe to revert it to the way it was, and even safe to remove the line complete.
| 'app/**/*', // Main application code | ||
| 'shared/**/*', // Shared utilities and components | ||
| 'ui/**/*', // UI components and styles | ||
| ]; |
There was a problem hiding this comment.
ENTRYPOINT_PATTERNS are just directories now, not globs, with no filters (IGNORE_PATTERNS), as they aren't needed.
Comment on lines
+60
to
+61
| // 'development/', // Development scripts and utilities | ||
| // 'test/', // Tests |
There was a problem hiding this comment.
It'd be nice to include these, but not necessary to see performance benefits.
| import { | ||
| OffscreenCommunicationTarget, | ||
| TrezorAction, | ||
| } from 'shared/constants/offscreen-communication'; | ||
| import type { Provider } from '@metamask/network-controller'; | ||
| } from '../shared/constants/offscreen-communication'; |
There was a problem hiding this comment.
Previously, madge would find this file, and try to import this file, which doesn't exist at the given location, and would just ignore it (it marks it as skipped, which this PR no longer allows)
Side note: Our typescript lint process seems to ignore this file.
Builds ready [6fa08fb]
Page Load Metrics (1704 ± 87 ms)
Bundle size diffs
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note to QA testers. This file does not change runtime behavior. It is a linting tool only. You do not need to test it for a release.
This PR prevents circular dependencies from being added to our codebase in the specified directories (see
.madgerc).It also fixes an issue with our previous version of this check. The previous version ignored many files, because it tried to use a non-existent webpack config. We no longer use the
webpackConfigoption (it doesn't do anything for us anyway), and we now have an additional check to ensure we do not skip tracking any imported files.We now check a list of folders that are allowed to have circular dependencies. If a circular dependency is added that it not allowed, the
[check](circular-deps:check)job fails, even if it was added to the./development/circular-deps.jsoncfile.It also does a check to make sure that all "allowed" files/folders actually have circular dependencies within them. If a user removes the last circular dependency from an "allowed" file/folder, the check fails (with a celebratory message and instructions on how to resolve the issue).
Example failure outputs for
yarn circular-deps:check:User needs to run
yarn circular-deps:update:User introduced a new circular dependency in a folder that isn't permitted:
User removed the last circular dependency in a file/folder that is allowed circular dependencies. They need to remove the allowed folder/file from .madgerc:
User introduced a file madge can't parse:
There is currently no good resolution for this error. We may need to add a way of working around this in the future. This check exists to ensure we don't skip any circular dependencies again.
Note: behavior of
yarn circular-deps:updateis not changed; it still only tracks circular deps, it doesn't care about our "allowed" directories or "skipped" files.To test:
To trigger the new "New circular dependencies issues were found in disallowed folders" error:
Remove any folder/file in the
allowedCircularGlobarray in.madgerc, runyarn circular-deps:update, then runyarn circular-deps:checkto generate the new error message.To trigger the new "The following allowed circular dependency patterns do not match any files" check:
Add a new file/folder (it doesn't need to exist) to the
allowedCircularGlobarray in.madgercthen runyarn circular-deps:checkto generate the new error message.To trigger the new "✖ Skipped files found" error:
Add an import to a file that madge can't parse, like a scss file (
import '../ui/css/index.scss';). You can test this by addingimport '../ui/css/index.scss';to/app/scripts/load/ui.ts, then runningyarn circular-deps:update.