Stacks.js: Remove circular dependencies

Created on 18 Jun 2020  路  2Comments  路  Source: blockstack/stacks.js

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.

bug P2

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?

All 2 comments

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:

  1. https://www.youtube.com/watch?v=CpLOm4o_FzM&ab_channel=Angular
  2. https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de
  3. https://stackoverflow.com/questions/24444436/circular-type-references-in-typescript
    These works to solve dependency in two files but in case of multiple files a lot of refactoring required that may change whole package structure as i explained in case of transactions package.

@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?

Was this page helpful?
0 / 5 - 0 ratings