Skip to content
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.

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

Distinguish CoreModule from AppModule or drop it #17825

Closed
dancancro opened this issue Jun 29, 2017 · 10 comments
Closed

Distinguish CoreModule from AppModule or drop it #17825

dancancro opened this issue Jun 29, 2017 · 10 comments
Assignees
Labels
feature Issue that requests a new feature freq2: medium state: has PR
Milestone

Comments

@dancancro
Copy link

I'm submitting a ...


[ ] Regression (behavior that used to work and stopped working in a new release)
[ ] Bug report 
[ ] Feature request
[X] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

The distinction between the roles of AppModule and CoreModule is unclear.

What is the motivation / use case for changing the behavior?

If the roles are identical - provision of app-wide singletons - then there ought not be two of these and CoreModule should be eliminated. It's just confusing. If the roles are different, the distinction
should be made perfectly clear. From my cursory survey on gitter, it seems that CoreModule
serves only to keep AppModule from getting too big, but this purpose is not at all stated in the Angular docs. If this is in fact its intended and only unique purpose, then it should be called something like "KeepAppModuleFromGettingTooBigModule". "CoreModule" implies that its constituent classes serve a core role that is different from the role of those in the AppModule.

  1. It is confusing to have two things that seemingly serve the same purpose and have no discernible advantages over each other.
  2. Many respectable example applications have no CoreModule.
@PoiScript
Copy link

there seem to be two definitions for CoreModule now..
one is:

Consider making CoreModule a pure services module with no declarations.

This page sample departs from that advice by declaring and exporting two components that are only used within the root AppComponent declared by AppModule. Someone following this guideline strictly would have declared these components in the AppModule instead.

the other one is:

Do gather application-wide, single use components in the CoreModule. Import it once (in the AppModule) when the app starts and never import it anywhere else. (e.g. NavComponent and SpinnerComponent).

@kapunahelewong
Copy link
Contributor

Thanks for this @dancancro. We've been working on re-thinking the presentation of the NgModules related documentation and the CoreModule topic confused me. The PR is big, so see the Singleton Services doc in progress.

If you have ideas on clarifying further, please feel free to comment #20306.

@frederikprijck
Copy link
Contributor

frederikprijck commented Jan 4, 2018

I've always looked at CoreModule vs SharedModule from the point of DI and LazyLoading (taking into acount that a lazy loaded module has it's own DI container), where a CoreModule only contains providers and is imported into the AppModule.

SharedModule on the other hand contains anything but providers (unless there's certain providers which should not be singleton in the case of LazyLoading).

In most libraries (aswell as the Angular Router) I see them being implemented as forRoot and forChild modules. (where forRoot includes the providers in the AppModule (which sounds abit like the CoreModule definition), and forChild acts as how SharedModule is defined in the FAQ. (there's other libraries doing the same, but with different method names such as ngrx).
In this case, both the so-called CoreModule (ModuleX.forRoot) and SharedModule (ModuleX.forChild) are the exact same module, but with different stuff being imported (thanks to ModuleWithProviders).

Note: This might not be 100% the same as the Core- and SharedModule definitions, as forRoot mostly includes the declarations aswell, but the idea behind it sounds the same to me (all about DI).

Looking at this, I'm not sure if there's a need for an explicit Core and SharedModule, in practice I often see multiple modules which either act as a Core- (providers for the root) or SharedModule (declarations, pipes and none-singleton providers) in a single (probably larger) project.

Don't get me wrong, I do think the idea behind both CoreModule and SharedModule is important, not sure if there's to be only one of them (nor do they have to be named Core or SharedModule), which is exactly what people are doing coz the docs mention them explicitly like that, resulting in big Core- or SharedModules.

I remember starting with Angular, being told I need a CoreModule and SharedModule (without a real reason why).
Once I managed to understand how DI works, it started to understand where they are coming from.

When I explain both CoreModule and SharedModule, I mostly start with explaining how DI works in case of LazyLoading and how both of them are useful in this situation (by explaining the problems, and how they are solved by both of the Module "types").


I've also seen CoreModules which act as a Module to keep AppModule clean, making AppModule only responsible for bootstrapping the App. Imho this is a nice idea, but it's something unrelated to the above CoreModule definition.

@dancancro

If the roles are identical - provision of app-wide singletons - then there ought not be two of these and CoreModule should be eliminated.

I don't think CoreModule is the same as AppModule, as you're also using so-called CoreModules when importing certain modules in ur AppModule (they just aren't named CoreModule). However, I do think naming it CoreModule explicitly is confusing, coz it makes people think they do need 1 so called CoreModule per application, while you might be having multiple so-called CoreModules in a real application.

@ngbot ngbot bot added this to the needsTriage milestone Feb 28, 2018
@jenniferfell
Copy link
Contributor

Current text makes a distinction/definition here:
"CoreModule is a conventional name for an NgModule with providers for the singleton services you load when the application starts."
https://angular.io/guide/ngmodule-faq#coremodule

I didn't review all text around other uses of CoreModule to make sure we're clear in those instances about it being a convention, but some were clear about this. Issue possibly resolved when we merged ngModules update earlier this year.

Keeping open to double-check.

@frederikprijck
Copy link
Contributor

frederikprijck commented Jun 4, 2018

Fwiw the CoreModule and ShareModule definition used on AIO (https://angular.io/guide/ngmodule-faq#coremodule) are clear to me.

However, it can still be a bit hard to understand for people not familiar with the reasoning behind it.

Afaik both the CoreModule and SharedModule come from a DI perspective in combination with LazyLoading. I currently see quite some developers blindly using the CoreModule/SharedModule approach without knowing why.

I agree, what I'm talking about is linked at the AIO page regarding CoreModule/SharedModule, but it's not that easy to get the full picture without collecting bits and pieces (https://angular.io/guide/ngmodule-faq#q-why-bad, https://angular.io/guide/singleton-services, https://angular.io/guide/ngmodule-faq#what-is-the-forroot-method ) from a few places.

@DeborahK
Copy link
Contributor

DeborahK commented Nov 2, 2018

Consider reviewing this topic for v6. Now with the providedIn feature of the service itself, it seems that the CoreModule is much less useful than prior to v6.

Consider removing the discussion and recommendation of a CoreModule entirely, especially from the Style Guide, and instead recommend using providedIn.

@Dali2579
Copy link

Consider reviewing this topic for v6. Now with the providedIn feature of the service itself, it seems that the CoreModule is much less useful than prior to v6.

Consider removing the discussion and recommendation of a CoreModule entirely, especially from the Style Guide, and instead recommend using providedIn.

I was in the way to ask you this question in your PS course. happy to see your answer here.

@kapunahelewong
Copy link
Contributor

Remove CoreModule PR has been merged. Closing this issue. @dancancro if you still have any questions or want to share feedback, please feel free to reopen and tag me.

@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Issue that requests a new feature freq2: medium state: has PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants