feat(@angular/cli): add circular dependency detection

[filipesilva]

Circular dependencies, like app.module.ts importing app.component.ts which in turn imports app.module.ts, now display a warning during builds:

kamik@T460p MINGW64 /d/sandbox/master-project (master)
$ ng build
Hash: 3516b252f4e32d6c5bb8
Time: 8693ms
chunk    {0} polyfills.bundle.js, polyfills.bundle.js.map (polyfills) 160 kB {4} [initial] [rendered]
chunk    {1} main.bundle.js, main.bundle.js.map (main) 5.95 kB {3} [initial] [rendered]
chunk    {2} styles.bundle.js, styles.bundle.js.map (styles) 10.5 kB {4} [initial] [rendered]
chunk    {3} vendor.bundle.js, vendor.bundle.js.map (vendor) 1.88 MB [initial] [rendered]
chunk    {4} inline.bundle.js, inline.bundle.js.map (inline) 0 bytes [entry] [rendered]

WARNING in Circular dependency detected:
src\app\app.module.ts -> src\app\app.component.ts -> src\app\app.module.ts

WARNING in Circular dependency detected:
src\app\app.component.ts -> src\app\app.module.ts -> src\app\app.component.ts

It is important to detect and eliminate circular dependencies because leaving them in might lead to Maximum call stack size exceeded errors, or imports being undefined at runtime.

To remove these warnings from your project you can factor out the circular dependency into a separate module.

For instance, if module A imports foo from module B, and module B imports bar from module A, it is enough to extract foo into module C.

You can turn off these warnings by running ng set apps.0.hideCircularDependencyWarnings=true. This will add the "hideCircularDependencyWarnings": true value to your .angular-cli.json and disable the warnings.

Fix #6309
Fix #6739

[Brocco]

LGTM, thanks for adding the option to disable/suppress this check.

[Splaktar]

This is good stuff! However, if I do the following steps, I get an error:

1. ng new tmp
2. cd tmp
3. ng serve -> yay it works
4. ng eject
5. npm i
6. npm start -> boom

> tmp@0.0.0 start /Users/splaktar/Git/angular/tmp
> webpack-dev-server --port=4200

module.js:471
    throw err;
    ^

Error: Cannot find module 'circular-dependency-plugin'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/splaktar/Git/angular/tmp/webpack.config.js:4:34)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)

Running npm i --save-dev circular-dependency-plugin seems to fix it, but ideally after ng eject things should 'just work'.

[filipesilva]

@Splaktar you're right, I forgot to add it to the list of dependencies that are added to the package.json. Will do it in a followup PR.

[colas74]

I wonder why I get this warning if I just import classes in order to have types, I'm using cli (v1.3.0-rc.0)

export abstract class AbstractLockableEntity {
  protected abstract formLocker : FormLockerComponent;
...

and in another file I have :

@Component(...)
export class FormLockerComponent {
  @Input() formInstance : AbstractLockableEntity   = null;
...

This get me:

Circular dependency detected:
lockable-entity.abstract.ts -> form-locker.component.ts -> lockable-entity.abstract.ts

cc @Brocco @filipesilva

[elvisbegovic]

@colas74 weird... it seems to me in java this is allowed, donno if it's cli or ts error.

[filipesilva]

@colas74 you have a warning because the imports you are using as circular. It doesn't really matter if it's for types or not (as far as detection is concerned).

@istiti you're absolutely right that it's allowed in many languages, and it's perfectly allowed in JavaScript as well. It's just that sometimes Webpack has problems with it (like the ones I marked fixed in the PR).
So the warnings are mostly to let you know about something that the build system itself (not the language) has problems with.

If you don't have any problems with your imports, feel free to go ahead and disable the warnings. But if you come across those errors, you can enable the warnings again and that might give you and idea of where the problem is.

[colas74]

@filipesilva Thx for your answer. I think there is no problem to import types and create circular dependencies, if they are only used for object's type. In any case, it is not seems to create performances issue

[elvisbegovic]

According to:

So the warnings are mostly to let you know about something that the build system itself (not the language) has problems with.

Javascript will works , but build system can have problem? So it's maybe one reason why ng build --prod Thake a while at 92% ?

[filipesilva]

The 92% thing is webpack resolving sourcemaps and I think module names? Maybe that's also when it actually assembles the bundles. To be honest I'm not 100% sure what's it's doing at that phase I just know it takes a while. It's outside the CLI control per se.

[mizi-lin]

Can't add property hideCircularDependencyWarnings, object is not extensible

[dwulfde]

@amily4555 try ng set defaults.build.showCircularDependencies=false
The option was moved

[rubenCodeforges]

And i guess it is now a issue.

Let assume we have this classes :

@Injectable()
export class ClassA {
    public static readonly STATIC_CONST_VALUE: string = "ILL NEVERE CHANGE";
    
    consturctor(private classB: ClassB){}
    
    public illCallClassBMethod(): void {
        this.classB.doSomething();
    }
}

export class ClassB {
    public doSomething() {
        // Some logic that requires a static field from classB
        console.log(ClassA.STATIC_CONST_VALUE);  //this will be now a circular dep, but its just a call to a class not a instance
    }
}

This will throw a warning now , i guess the way the plugin works , its checking the imports.
Which is not correct, the ClassA.STATIC_CONST_VALUE is a static field and should be accessed from everywhere without a problem and is not violating any OOP or DI rules.

The feature is good , no doubt , but i would suggest to turn it off by default.

[angular-automatic-lock-bot]

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.}