Describe the bug
Many build systems report circular dependencies as warnings or errors. We should remove these errors, and add a CI job to prohibit them. In many cases, this simply requires breaking up files.
ERROR in Circular dependency detected:
src/auth/authApp.ts -> src/auth/authMessages.ts -> src/auth/userSession.ts -> src/auth/authApp.ts
ERROR in Circular dependency detected:
src/auth/authMessages.ts -> src/auth/userSession.ts -> src/auth/authMessages.ts
ERROR in Circular dependency detected:
src/auth/userSession.ts -> src/auth/authMessages.ts -> src/auth/userSession.ts
ERROR in Circular dependency detected:
src/config.ts -> src/network.ts -> src/logger.ts -> src/config.ts
ERROR in Circular dependency detected:
src/logger.ts -> src/config.ts -> src/network.ts -> src/logger.ts
ERROR in Circular dependency detected:
src/network.ts -> src/logger.ts -> src/config.ts -> src/network.ts
ERROR in /Users/kyran/dev/blockstack/blockstack.js/src/profiles/makeProfileZoneFile.ts
./src/profiles/makeProfileZoneFile.ts
We can use the CircularDependencyPlugin for webpack to report these.
Findings on this so far:
Used https://www.npmjs.com/package/madge package to list the dependencies which results in following output:
1) cli/src/auth.ts > cli/src/data.ts > cli/src/utils.ts > cli/src/keys.ts > cli/src/cli.ts
2) cli/src/data.ts > cli/src/utils.ts > cli/src/keys.ts > cli/src/cli.ts
3) cli/src/keys.ts > cli/src/cli.ts
4) cli/src/utils.ts > cli/src/keys.ts > cli/src/cli.ts
5) cli/src/utils.ts > cli/src/keys.ts
6) keychain/src/identity.ts > keychain/src/profiles.ts
7) keychain/src/identity.ts > keychain/src/utils/index.ts
8) transactions/src/authorization.ts > transactions/src/keys.ts
9) transactions/src/authorization.ts > transactions/src/keys.ts > transactions/src/types.ts
10) transactions/src/keys.ts > transactions/src/types.ts
11) transactions/src/types.ts > transactions/src/postcondition.ts
12) transactions/src/clarity/clarityValue.ts > transactions/src/clarity/types/principalCV.ts
From the above list of issues, only the issues that involve only two interdependent files seems to easily refactorable by moving the interdependent code in one file. For example in case of point 12 if we move principalToString function from transactions/src/clarity/types/principalCV.ts to transactions/src/clarity/clarityValue.ts it will solve the issue 12.
But the issues that involve multiple files in loop like files authorization.ts, keys.ts, types.ts, postcondition.ts are making a loop to use the code from each other (The points 8-9-10-11 are in strong loop) not seems to be easily refactorable because removing one dependency breaks lot of code in other files and creates more circular dependencies if i try to move the code to separate common file.
I tried to use concepts explained in below links:
https://www.youtube.com/watch?v=CpLOm4o_FzM&ab_channel=Angularhttps://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0dehttps://stackoverflow.com/questions/24444436/circular-type-references-in-typescript@kyranjamie Let me if you have any thoughts to achieve this by minimal refactoring or is it worth to do major refactoring to solve these dependencies.
Curious to get input from others here @zone117x @reedrosenbluth @blockstack/ux-team
IMO these circular dependencies highlight a lack of consideration given to the relationship between these packages/files within packages, so do think we should make an effort to remove these (or risk facing issues greater complications later).
I haven't tested this, but in some cases it might also be that we need to use import type {} instead of a straight import.
Perhaps the scope of this issue could be narrowed to first fixing the easy circular dependencies, and adding a CI job with a warning, rather than an error?
Most helpful comment
Curious to get input from others here @zone117x @reedrosenbluth @blockstack/ux-team
IMO these circular dependencies highlight a lack of consideration given to the relationship between these packages/files within packages, so do think we should make an effort to remove these (or risk facing issues greater complications later).
I haven't tested this, but in some cases it might also be that we need to use
import type {}instead of a straight import.Perhaps the scope of this issue could be narrowed to first fixing the easy circular dependencies, and adding a CI job with a warning, rather than an error?