10000 Angular Circular Dependencies - removing showCircularDependency flag has resulted in problems for big enterprise applications · Issue #24870 · angular/angular-cli · GitHub
[go: up one dir, main page]
Skip to content

Angular Circular Dependencies - removing showCircularDependency flag has resulted in problems for big enterprise applications #24870

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

Jump to bottom
Closed
ajitprak opened this issue Mar 16, 2023 · 4 comments
Closed

Angular Circular Dependencies - removing showCircularDependency flag has resulted in problems for big enterprise applications #24870

ajitprak opened this issue Mar 16, 2023 · 4 comments

Comments

@ajitprak
Copy link

Which @angular/* package(s) are relevant/related to the feature request?

No response

Description

Angular has removed the showCircularDependency flag. I understand it is for performance improvement, But for already existing big enterprise-level applications, with many developers, it poses a big problem. Since many devs are working, an import-based circular dependency can be easily inserted in any part of the code, and with the useful 'circular dependency warnings' removed, there is no way to find such issues. And these issues may cause application to actually break in production. Which causes a huge impact on our reputation with the customer. We tried the alternatives suggested, but no luck

  1. Eslint -> import/no-cycle -> Makes our linter in CI run for more than 1.5 hours and finally breaks
    14:55:00 Linting "@abcd/mnop-efgh"...
    16:27:07 Cannot contact xyz-slave1: java.lang.InterruptedException
    This is probably because our application is very big

  2. Madge -> Only found the type-based circular dependencies and no other circular dependencies... It is able to detect all circ-deps for a smaller application, But since our app is huge it is probably not able to find anything relevant.

Proposed solution

  1. Keep the showCircularDependencies flag for development and not have it in production. In the angular.json we can have a warning, if showCircularDependencies is set to true, stating, "Do not use in Production as it will slow the build".

  2. Angular can have another script, which after running can let us know the circular dependency warnings. This script can be based on the build, used in Angular-13 or before... which finds circular dependencies. This script will be different from the build so no performance impact will be there.

Alternatives considered

As mentioned above eslint and madge were tried, but no value was added.
@alan-agius4 alan-agius4 transferred this issue from angular/angular Mar 16, 2023
@alan-agius4
Copy link
Collaborator

Hi,

The problem with showCircularDependencies was that it check came with an enormous performance penalty, even in for development during incremental rebuilds.

Circular dependencies is more suited for a linter/external tool than a build step. One point to consider is that cycles dependencies in ES modules are allowed.

If Eslint circular dependency checker is slow is suggest you file an issue with the other of the rule.

@alan-agius4 alan-agius4 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 21, 2023
@ajitprak
Copy link
Author

HI @alan-agius4 ... Thanks for responding...
I agree with the performance penalty point of view ...
How about my second proposed solution ... Creating a script that finds circular dependencies ... You could use the same code that spits out circular dependency warnings(In Angular-13) ... This script can be separate from the build, so it does not have any performance impact in dev or prod...
It will be very useful for already-built enterprise-level applications, helping them foresee unexpected breaking of their app.
You can even point us to the code(with some details), and we can create that separate script ... It will be useful for us and other enterprise apps.

@vbraun
Copy link
vbraun commented Mar 30, 2023

We are using madge successfully, it does find all circular dependencies. I have two notes on its use:

A) type-only circular imports are OK, you can ignore these with

$ cat .madgerc 
{
    "baseDir": ".",
    "detectiveOptions": {
        "ts": {
            "skipTypeImports": true
        }
    }
}

B) madge only emits warnings when the imported file is not found (can be made more verbose with madge --warning)

Processed 18 files (515ms) (72 warnings)
                            ^^^^^^^^^^^

but import checking obviously stops at the file that was not found. You need to run madge with a tsconfig file that has all path mappings so the imported files can be found (i.e. no warnings)

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@vbraun @ajitprak @alan-agius4
0