Circular dependency error with service generated with --module in 6.0.0-rc.0

[cyrilletuzi]

Versions

Angular CLI: 6.0.0-rc.0
Node: 8.11.0
OS: darwin x64
Angular: 6.0.0-rc.1
... animations, common, compiler, compiler-cli, core, forms
... http, language-service, platform-browser
... platform-browser-dynamic, router

@angular/cli: 6.0.0-rc.0
@angular-devkit/architect: 0.0.10
@angular-devkit/build-angular: 0.0.10
@angular-devkit/build-optimizer: 0.4.9
@angular-devkit/core: 0.4.9
@angular-devkit/schematics: 0.5.0
@ngtools/json-schema: 1.1.0
@ngtools/webpack: 6.0.0-beta.9
@schematics/angular: 0.5.0
@schematics/update: 0.5.0
typescript: 2.7.2
webpack: 4.1.0

Repro steps

• ng new hello
• ng g module world --module app
• ng g service world/data --module world
• ng g component world/page
• inject the service in the component constructor

Observed behavior

WARNING in Circular dependency detected:
src/app/world/data.service.ts -> src/app/world/world.module.ts -> src/app/world/page/page.component.ts -> src/app/world/data.service.ts

Desired behavior

CLI v6 ng g service command now provides the service in a tree-shakeable way, using the providedIn option of Injectable decorator.

Without the --module option, it uses providedIn: 'root', which seems OK.

But with the --module option, CLI v1 was providing the service directly inside the NgModule. Now in CLI v6, the module is configured via providedIn. But then the service is not usable in components, which is a serious problem.

I see 2 options :

• either it is an Angular issue, which should be resolved ;
• either it is a known limitation of providedIn, then the CLI should keep its old behavior of providing the service directly in the NgModule when using --module.

[filipesilva]

That's a good point. I'm asking around if this is by design. It's worth noting that the circular dependency detection was introduced in the past because there were cases where it broke bundling. Perhaps it is not actually needed anymore.

[about-code]

It's worth noting that the circular dependency detection was introduced in the past because there were cases where it broke bundling. Perhaps it is not actually needed anymore.

Please, "don't shoot the messenger". In fact it seems to me the circular dependency warning is a life-saver here, pointing out on an otherwise very hard to detect problem. Dropping the message won't solve the problem:

Now in CLI v6, the module is configured via providedIn. But then the service is not usable in components [...].

The service is obviously not being injected in case of a circular dependency. Actually, that sounds like a problem with the angular compiler who reasons about decorator metadata for dependency injection and who seems to have issues to properly resolve those circular dependencies - it could also be a runtime problem in the code the compiler generates (you guys probably know better than me).

I would expect much more people to run into this once tree-shakable providers will be released for two reasons:

• service--[provides itself to]-->module--[declares]-->component--[depends on]-->service is going to be a very common pattern because in the majority of cases it makes just sense that modules and their components consume their own services

• there will be many people who will think gaining tree-shaking efficiency with DI means to do things just the other way around. So many will probably begin to scan their MyNgModule providers array and systematically add @Injectable({providedIn: MyNgModule}) to any service they find before dropping the array.

Unless there's a solution to this problem providedIn: 'root' will mostly be the only viable option to choose when using tree-shakable providers, I guess.

[hansl]

service--[provides itself to]-->module--[declares]-->component--[depends on]-->service is going to be a very common pattern because in the majority of cases it makes just sense that modules and their components consume their own services

Please stop teaching or telling pepole to use this pattern. With tree shakeable providers, you now have zero reasons to provide a service from a module (there is a rare case on some lazy loaded services with a component using the service from a separate lazy module). The --module flag is there as a courtesy, but there's a reason it's not the default; it shouldn't be used.

providedIn: 'root' is the 99% of cases.

What will likely happen is; we will either

1. remove the --module option entirely, or
2. show a message that it's probably not what you want when you use that option.

People using --module without knowing what they're doing need to see the circular dependency error and tell themselves "that's odd", and search and find that they shouldn't use it. Hopefully we'll stop using this pattern entirely in courses / tutorials / examples.

[cyrilletuzi]

@hansl Thanks for the explanation.

I think deleting the --module option right now would be the right way to go. Indeed, before v6, we had to provide services in NgModule providers, which was done with the CLI with --module. So people will first continue to use --module but suddenly in V6 it will do a new behavior which you say is bad and leads to errors.

[about-code]

service--[provides itself to]-->module--[declares]-->component--[depends on]-->service is going to be a very common pattern because in the majority of cases it makes just sense that modules and their components consume their own services

Please stop teaching or telling pepole to use this pattern

@hansl I think we think pretty much the same about the issue. I didn't intent to teach people to use this pattern nor should above "pattern" be understood as a recommendation. Maybe I should have called it anti-pattern

People [...] need to see the circular dependency error and tell themselves "that's odd", and search and find that they shouldn't use it.

In fact that's what I argued for, too. My comment was meant as a response to the thoughts of @filipesilva who thought whether it were possible to drop the circular dep. detection alltogether. I think it provides an invaluable hint on the cause of @cyrilletuzi's issue and points at code smells which are unpleasant and unintended most of the time.

By the current typings and docs it is at least technically possible to use providedIn with a module type which I think will look to many people like optimizing for tree-shakability means you can make things work by just doing them the other way around, compared to v5 which is obviously not the case.

Therefore I am afraid there will be many people who migrate their code and end up with the (anti-)pattern I described. Without any warning they would only find out that "for some reason" the service simply won't be provided to some components. They would face a problem which is very hard to debug. Seeing the circ. dep. from just the code is not any easier. And therefore I came to the same conclusion as you did. The circular dependency warning might not tell people directly that they've used @Injectable the wrong way, but it indicates that there's something fishy and from the path it shows it is not that hard to see what the problem is.

providedIn: 'root' is the 99% of cases.

That was my impression, too. Given the docs 'root' should be default if providedIn was not declared at all. So if any collegue of mine would ask me about any recommendation regarding providedIn I would probably recommend to only use it if not using it doesn't work ;-)

[larssn]

You might wanna strip the following from the docs, to not give people any good ideas:

import { Injectable } from '@angular/core';
import { HeroModule } from './hero.module';
import { HEROES }     from './mock-heroes';

@Injectable({
  // we declare that this service should be created
  // by any injector that includes HeroModule.

  providedIn: HeroModule,
})
export class HeroService {
  getHeroes() { return HEROES; }
}

Source: https://angular.io/guide/dependency-injection#injectable-providers

Or keep it, and put a very big fat warning box next to it.

[localyost3000]

I'm experiencing this problem as well, but am unconvinced that providedIn: 'root' is a good longterm solution for every service. For instance, I was under the impression that lazy-loaded modules bundled their provided services with them, meaning this new method would swell the app or common bundle considerably. If that's not the case, then I guess I'm on board, but I'd be surprised to learn that.

Since 'root' is a string Token to look up the root module, why not allow us to declare module selectors in the same vein? That way you could do providedIn: 'UsersModule' without the explicit import of UsersModule which trips the circular dependency detection?

[fxck]

Encountered this as well.

Api service imports module, module imports ngrx/effect, ngrx/effects imports api service.

So I should just use provideIn: 'root' no matter what?

[wardbell]

Whoa Nellie!

I have a large number of services that are scoped to modules (lazy modules) or even to components within those (lazy modules).

This pattern has worked GREAT for a very long time.

I understand the desire to make LIBRARIES treeshakable. A library often includes tons of code that many consumers will not need or want.

However, APPLICATIONS typically contain very few treeshakable services. If I write a service for my app, it's because I expect the app to need it. I don't write app services that someone might NOT need.

Therefore, treeshaking is of very little benefit to application code.

If you think I'm wrong, show me (us) please.

FWIW, I know we can use the new syntax to declare that a service is provided in a non-root module (and I think it works in a component too). That's grand, but do I need it? In an application, I don't think that I do.

Moreover, this style is a mixed blessing. On the plus side, it's easy to tell by reading the service where it is provided and to ensure that it is provided there. On the negative side, I've pinned the service to that particular usage. I can't safely provide it at another level because I'm at risk of service shadowing (a serious problem for a stateful service).

It feels premature to make such bold statements about a feature that none of us could have used for more than a few months.

[sliekens]

People using --module without knowing what they're doing need to see the circular dependency error and tell themselves "that's odd", and search and find that they shouldn't use it.

Well you totally busted me right now.

Since 'root' is a string Token to look up the root module, why not allow us to declare module selectors in the same vein? That way you could do providedIn: 'UsersModule' without the explicit import of UsersModule which trips the circular dependency detection?

+1 for this idea.

@NgModule({ selector: 'heroes' })
export class HeroModule {}

@Injectable({ providedIn: 'heroes' })
export class HeroService {}

I think that's a nice way to do it. This should be possible, right?

[clydin]

Tree shaking providers are opt-in. The classic method of providing the services via NgModule metadata is still viable if desired (just remove { providedIn: 'root' } if doing so).

[localyost3000]

As an update to my last post: it does appear that even when providedIn: 'root' webpack will bundle the service with the correct lazy module bundle, so no concern there.

[sliekens]

I'm lost on what is the best practice here... 🤔

I think re-export HttpClientModule from your lazy-loaded module (but don't shoot me if I'm wrong).

@NgModule({
    imports: [HttpClientModule],
    exports: [HttpClientModule]
})
export class LazyLoadedModule {
}

@Injectable({ providedIn: 'root' })
export class LazyService {
    constructor(private http: HttpClient) {
    }
}

[luchsamapparat]

@StevenLiekens no, that didn't help :-/

[about-code]

@luchsamapparat I can not provide some "official" recommendation but I think the best practice in cases where tree-shakable providers don't work is to continue providing things as they have been provided prior to tree-shakable providers. The v5 mechanics haven't been deprecated.

Nevertheless, did you try the pattern shown by @manfredsteyer in angular/angular#23715. It's far from pretty but might help with tree-shakable providers.

[stefanzvonar]

After playing with this for the last hour, I have come to the following conclusion that for services used across the application, then it is only useful to have providedIn: 'root'. Do not bother putting these into the providers array of the AppModule.

However, if you want to provide a service in any feature module (not the root), then you are better off using the providers array in the feature module's decorators, otherwise you will be plagued with circular dependency warnings.

[sliekens]

@StevenLiekens no, that didn't help :-/

Then my other recommendation is importing HttpClientModule in your app module.

Why should I declare a dependency here that is required by a service in a lazy-loaded module?

Because your app module is the composition root, which is where you should register all services that you will eventually use. Think of HttpClient as a dependency of your app, not as a dependency of your lazy module.

Or to put it in NPM terms: HttpClient is a peer dependency between your app and your lazy modules.

[sliekens]

My LazyLoadedModule has an import on HttpClientModule, but AppModule doesn't. Why should it?

Here's why that breaks providedIn: 'root':

When the Angular router lazy-loads a module, it creates a new injector. This injector is a child of the root application injector.

https://angular.io/guide/providers#limiting-provider-scope-by-lazy-loading-modules

Okay so what this means is that your lazy module gets its own private copy of HttpClient. The root module doesn't know about your private HttpClient so the root injector shits all over the console when you try to get an instance of a service that depends on HttpClient.

I do feel like re-exporting the HttpClientModule should have added HttpClient to the root injector but maybe my expectations are wrong.

[luchsamapparat]

For me it feels like this feature, while nice in theory, isn't thought through…

[shairez]

From reading lots of things about the topic, what I understand is the following:

Like @hansl wrote: "Always use providedIn: 'root'

Like @swimmadude66 wrote: it will still be bundled in the right bundle (so they the build does the job for you if it's only used in the lazy loaded bundle)

And just like in this issue 23715 - If you really want to limit the scope, you can do it with a separate module for services which gets loaded by the lazy loaded module.

The only thing missing is to update the docs and I think it'll be better to remove the section that shows you can use providedIn with specific modules (or just hide it in a "super rare cases" section).

I think that will reduce the confusion and will promote the better practice. (FYI @jenniferfell )

CONCLUSION:

Always use the providedIn: 'root', (for both eager + lazy loaded modules' services)

1. it's better,
2. it solves the "duplicated services instance" problem on shared lazy loaded modules
3. It's easier to learn and to use

[sliekens]

So back on topic: what should the --module flag do? Old or new behavior, or kill that switch entirely? And what about the issues with providedIn: 'root' discovered by luchsamapparat ?

[bh3605]

@StevenLiekens --module adds the service to the referenced module's providers array and doesn't include the providedIn: 'root'?

I have a feature module and I was trying to specify the module name in the providedIn and encountered circular dependency warnings like everyone else. Until you can tokenize module names as strings like what root references then just use the v5 method. That's what I'm going to do >.>

Sounds like we need a feature request to tokenize a module name to be used by services preventing the circular dependency warning. If this was implemented THEN --module could specify the module selector in the providedIn option.

[localyost3000]

It seems like if the obvious answer is that everything must be providedIn: 'root', that should just be the sane default, and Injectable should go back to having no arguments except in the rare case it needs an override.

[cedvdb]

If it should be providedIn: 'root' in virtually all cases then having to specify it only serve the purpose of confusing people.

[shairez]

@cedvdb I'm with you, if that was the only way to define providers.
But I guess they made us configure it this way not to break existing projects,
because the providedIn way does change the functionality slightly.

[hiepxanh]

add here for notation purpose. I take 3 hours to invest about this, and this blog: The New Treeshakable Providers API In Angular: Why, How And Cycles
explain for me very well.
the problem is:

we can resolve it by creating new api module for that service:

that all I need.

@Injectable({ 
    providedIn: FlightApiModule
})
export abstract class AbstractFlightService  {
}

In addition, the lazy module also needs to import the new service module:

@NgModule({
    imports: [
        [...]
        FlightApiModule
    ],
    [...]
})
export class FlightBookingModule {
}

[MickL]

Has everyone read the updated documentation yet? Also there is the FAQ providing tons of informations how it works (probably better than the article on softwarearchitect.io ?)

If i get this right:

1. Don't use providedIn: UserModule if you develop a application. It is only needed for developing libraries: When someone is using your library he cant provide your service if he didn't import UserModule
2. Mostly always use 'root'. The service is will be available application wide. If the service is only used within a lazy loaded module it will be lazy loaded with that module, if it is never used it will be tree shaked.
3. If you want to limit the scope of a service remove providedIn and provide it within your lazy loaded module the old fashioned way (preferred) OR provide it within a component
4. Note: Don't provide services by a shared module
5. Note: If you provide your service within a sub-module, it will by available application wide unless the sub-module is lazy loaded.

Can someone confirm?

[tomastrajan]

@MickL

5. Note: If you provide your service within a sub-module, it will by available application wide unless the sub-module is lazy loaded.

I think this is not completely correct. If you use providedIn: 'root' in your LazyService which you only want to use in LazyModule but you still inject LazyService in some eager part of your application it will be automatically bundled into eagerly loaded bundle not the lazy one.

This is from my POV a problem because you can't guarantee that something is not used somewhere where you don't want it to be used. The only way to guarantee that is to use providedIn: LazySubModule so that you avoid circular deps warning but also get error when injecting LazyService in the eager part of the application (which is desirable). Then last step is to import LazySubModule by the LazyModule and done.

[MickL]

With "5." i mean provide with the providers array. Do not use providedIn: LazySubModule. If you read the docs and faq you will see that this is how Angular works. The service will be available everywhere even tho it is provided only to the submodule, except the module is lazyloaded.

[tomastrajan]

@MickL I see that, so then it is a true statement to say that these two are equivalent?
@hansl

Use providers: []

@NgModule({ providers: [SomeService] })
export class SomeModule {}

• old style

• service will be global singleton if SomeModule is imported eagerly

• service will be instantiated per lazy module if SomeModule is loaded lazily

• injecting SomeService in eager part of application will result in error if SomeModule is loaded lazily

• DISADVANTAGE - mixing of the old providers: [] and the new providedIn syntaxes

Use providedIn

@Injectable({ providedIn: SomeSubModule })
export class SomeService {}

@NgModule({ imports: [SomeSubModule]})
export class SomeModule

• new providedIn syntax

• service will be global singleton if SomeModule is imported eagerly

• service will be instantiated per lazy module if SomeModule is loaded lazily

• injecting SomeService in eager part of application will result in error if SomeModule is loaded lazily

• DISADVANTAGE - extra module to prevent circular deps

[leandro-hl]

You might wanna strip the following from the docs, to not give people any good ideas:

import { Injectable } from '@angular/core';
import { HeroModule } from './hero.module';
import { HEROES }     from './mock-heroes';

@Injectable({
  // we declare that this service should be created
  // by any injector that includes HeroModule.

  providedIn: HeroModule,
})
export class HeroService {
  getHeroes() { return HEROES; }
}

Source: https://angular.io/guide/dependency-injection#injectable-providers

Or keep it, and put a very big fat warning box next to it.

This has just happend to me using this page:

https://angular.io/guide/providers#providing-a-service

Please guys could you remove it from de docs? Or add something that warning the people. I saw the problem after implement it.

[DanKing123]

Please correct me if I'm missing something, but it seems to me that adding the providedIn: 'root' syntax breaks the link between the service and the module it nominally lives in?

The service doesn't know about the module, and the module doesn't know about the service anymore, so the service is no longer really "in" the module. Instead, the service will just get bundled with whatever references it?

The result of this seems to be that we can no longer guarantee that the service's dependencies (as defined by the module it "lives in") will be loaded when the service is injected, hence - I think - the problem described by @luchsamapparat?

It seems to me this could be fixed using the providedIn: 'UserModule' idea, suggested by @swimmadude66 (which allows the module to be specified, without causing a circular reference).

Alternatively though, wouldn't it have been simpler not to have introduced the providedIn syntax in the first place? Tree shaking of components is already performed, even when they are referenced in the declarations and exports arrays, so couldn't this have been done for providers as well - before they're added (or not) to the injector - instead of introducing a new syntax...?

[danieldanielecki]

Most probably you wanted to have a service in 1 particular component/module. You can do it by doing the following:

@Injectable()
export class FooService {
  ...
}

And in your component:

import { FooService } from 'PATH_TO_THE_SERVICE';
@Component({
  selector: 'app-lazy',
  templateUrl: './lazy.component.html',
  styleUrls: ['./lazy.component.scss'],
  providers: [FooService]
})

Do not make it this way:

@Injectable({
  providedIn: LazyModule
})

It will show warnings about circular dependencies. Also do not import this in the providers array in the LazyModule, combined with the last code snippet will show the same warnings. Alternatively you can simply do the following:

@Injectable({
  providedIn: 'root'
})

Angular will then detect automatically in which component it has been used. If it wouldn't be used it will be tree shaked.

[jbeckton]

Angular contributor says make all your service instances Root scoped Singletons? Regardless of the programming language or framework this can't be a serious mindset. If I wanted to do silly things with code I would switch to React and become a JSX fan.

Being able to control the scope and lifetime of an injected dependency instance is very important, I hope the Angular Team is not planning on taking this capability away for tree shaking improvements. What you end up with is a small file bundle that quickly becomes bloated in memory at run time.

I don't use 'providedIn' because I like being able to control scope and lifetime of my service instances, and when a developer has this mindset then there is no need for tree shaking since I intend for my services to be used else I would not go to the trouble of providing them only where they are needed.

Since the CLI defaults to providedIn when generating a service class I see teams of Angular newbies using it everywhere in their apps for services that are written to work only with a single component instance. I would like to see the providedIn technique be Opt In and remove the {providedIn: 'root'} snippet from CLI generated services.

[MickL]

Why would you handle the scope of your services when providedIn already choses the perfect usage? If the service is used only with a single component then it is bundled with this component. If it is used as a app wide singleton then it is. The CLI always handles it perfectly no need to think about it again. I think this is great for newbies and for professionals. Still it is optional and you can handle it manually too so i dont see the problem.

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