build: Check for circular deps with madge #3631
Conversation
size-limit report
|
|
If a dependency is shared (like in this case), it can live in the root |
Fixed |
Given that it's its own job and that nothing depends on it, it should be able to run in parallel, so it actually shouldn't add any time to CI. |
|
Turning this into a bigger feature branch and merging in other PRs here. |
AbhiPrasad
force-pushed
the
abhi/circular-dep
branch
6 times, most recently
from
679e99a to
80f3962
Compare
Madge (https://www.npmjs.com/package/madge) is a library that can traverse module dependency trees to check for circular dependencies. We can leverage it using `madge --circular /path/to/index.ts` to make sure that our packages don't contain circular deps. On average madge takes around 2-3 sec, so we are not sacrificing a ton of CI time.
When running madge on `@sentry/node`: $ madge --circular src/index.ts Processed 10 files (698ms) (8 warnings) ✖ Found 1 circular dependency! 1) backend.ts > parsers.ts To remove the circular relationship between backend and parsers, we extract the NodeOptions type into a separate types.ts file.
fix adjust rule
``` $ madge --circular src/index.ts Processed 5 files (1s) (2 warnings) ✖ Found 2 circular dependencies! 1) hub.ts > interfaces.ts ``` To remove this, the interface.ts was removed and all of it's types were moved into hub.ts
``` yarn run v1.22.5 $ madge --circular src/index.ts Processed 5 files (1s) (2 warnings) ✖ Found 1 circular dependency! 1) hub.ts > interfaces.ts > scope.ts > session.ts ``` To remove this circular dep, we extract the SessionFlusher from session.ts into it's own file.
``` yarn run v1.22.5 $ madge --circular src/index.client.ts && madge --circular src/index.server.ts Processed 5 files (794ms) (7 warnings) ✔ No circular dependency found! Processed 7 files (798ms) (9 warnings) ✖ Found 1 circular dependency! 1) index.server.ts > utils/handlers.ts > utils/instrumentServer.ts ``` To remove the circular dependency between index.server.ts and utils/instrumentServer.ts, we rely on importing Sentry func (like startTransaction and captureException) from `@sentry/node` directly.
No need to check types files as they are not needed
AbhiPrasad
force-pushed
the
abhi/circular-dep
branch
from
bbc5691 to
e07e07c
Compare
|
Squashed, rebased commits. Each commit should be fairly self explanatory in terms of changes. |
|
You might check in with @k-fish about the failing ember test. I'm pretty sure the way it's failing is the same way it failed once before on a PR of mine, and he was able to figure out what the issue was and fix it. |
|
Oh, tiny nit for future:
s/it's/its |
|
@lobsterkatie ah right, we did see this before briefly, I think this one might be a flake though. It checks the exact spans that are sent for a pageload, and is just dropping the post render runloop spans, which makes sense if the transaction is wrapping up early. Since it could be a flake unique to how we are testing, I'd first try re-running for now. Just dm me if it continues to fail and I'll look into fixing it. |
AbhiPrasad
force-pushed
the
abhi/circular-dep
branch
from
ddf9ced to
3a03a7f
Compare
|
Thanks, @k-fish! Will let you know. |
Madge (https://www.npmjs.com/package/madge) is a library that can
traverse module dependency trees to check for circular dependencies.
We can leverage it using
madge --circular /path/to/index.tsto makesure that our packages don't contain circular deps. On average madge
takes around 2-3 sec per run, so we are not sacrificing a ton of CI time.