angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
94.62k stars 24.65k forks source link

Angular Module instance never get destroyed #37095

Open SukeshMarla opened 3 years ago

SukeshMarla commented 3 years ago

No matter what we do lazy loading or eager loading Angular Module instance never gets destroyed.

When we route across components or use *ngIF Components get destroyed and recreated but its not the same case when we navigate from one module to another.

While lazy loading every module gets loaded dynamically. Also, the instance get created but that instance never gets destroyed.

To check the same I put the breakpoint in Module constructor.

Anybody know the reason for such behavior. I feel it bad because all services registered on those modules get remain forever then too.

I felt like it a bug than a feature request.

mlc-mlapis commented 3 years ago

@SukeshMarla You have to destroy it yourself: https://angular.io/api/core/NgModuleRef It's generally hard to predict when such a module should be destroyed automatically.

SukeshMarla commented 3 years ago

@SukeshMarla You have to destroy it yourself: https://angular.io/api/core/NgModuleRef It's generally hard to predict when such a module should be destroyed automatically.

But it's quite a difficult job to do it manually. When we go away from a particular module, destroy it. Wondering how to get it done.

mlc-mlapis commented 3 years ago

@SukeshMarla If you know when you load a lazy loaded module, you also know when you leaving it. Each module can have an id. And also imagine what it means go away from a module. Also it's very unclear generally what is more effective, if load a module repeatedly or keep it in a memory. Probably it a case from case different.

SukeshMarla commented 3 years ago

@SukeshMarla If you know when you load a lazy loaded module, you also know when you leaving it. Each module can have an id. And also imagine what it means go away from a module. Also it's very unclear generally what is more effective, if load a module repeatedly or keep it in a memory. Probably it a case from case different.

Let's say I am navigating to route belongs to another module shouldn't previous module instance get removed from memory. Or else we end up with so many modules in memory.

Secondly, I may have many modules in the system.

Do we have any event inside modules which get fired when we are navigating away from it.

SukeshMarla commented 3 years ago

Any updated thought on this from anyone,

pkozlowski-opensource commented 3 years ago

When we route across components or use *ngIF Components get destroyed and recreated but its not the same case when we navigate from one module to another.

@SukeshMarla what do you mean exactly by "navigate from one module to another"? How is a module in question referenced? In a router? Or are you loading it separately? Is it a lazy-loaded module or not? What is the version of Angular that you are using? And when you say that NgModule is not destroyed - is it about the module instance or components / providers in this module?

I would have several more questions but it boils down to the fact that we need a minimal reproduce scenario to understand your situation. Could you please provide a minimal stackblitz or a GitHub repository showing that something is not destroyed, while it should be.

I'm sorry but the issue, as it stands right now, is impossible to understand and act upon.

SukeshMarla commented 3 years ago

ok, here it is a step by step instruction.

My point is we may have hundreds of feature modules in the system. If every module instance remains in the memory forever won't it produce performance overhead as some services must have registered on those modules and so their instance will be in the memory as well?

Any inputs on it appreciated.

SukeshMarla commented 3 years ago

anyone with any answer or thoughts.

mlc-mlapis commented 3 years ago

@SukeshMarla I was thinking about the theme meantime and I am still not sure how the full picture can be analyzed by relatively simple rules to have the clear decision if it's or it isn't possible to destroy the module instance. It's not possible to take only some variants, it's necessary to see all possibilities.

vishal4799 commented 3 years ago

As @SukeshMarla mentioned, performance overhead is surely a major issue, moreover, when we navigate back to submodule which has its own service (providedIn that submodule and not as 'root') still holding old data which causes inconsistency in an application. Of course, there are workarounds to refresh the data but we have to do it manually. It would be very simple and clear if the module instance gets destroyed along with the service instance when we navigate away and recreated again on navigating back.

jelbourn commented 3 years ago

Summarizing this issue so far: this seems to be working as designed. There's nothing in Angular that automatically destroys NgModules. However, it's potentially worth further discussion on whether it should.

Akxe commented 3 years ago

Hi, I just stumbled on this issue. I too think that modules should be destroyed once navigated away (via RouterModule). I do not see a clear point on why would one want to keep a module alive after navigating away from it.

How about solving it via extending the Route interface to include property like destroyOnNavigationAway:? boolean?

I do not really care about the module itself, rather I care about its services are not being destroyed. They do hold quite a large memory footprint in my case.

At this point, I do not know, if any service that is not scoped only to a component is ever going to get destroyed... (I would love to know actually :) )


Potential solution

A new option in Route interface destroyOnNavigationAway:? boolean:

@NgModule({
  declarations: [
    MyComponent,
  ],
  imports: [
    RouterModule.forChild([
      {
        path: '',
        component: MyComponent,
        pathMatch: 'full',
      },
      {
        path: 'inquiry',
        destroyOnNavigationAway: true, // This property
        loadChildren: () => import('./inquiry/inquiry.module').then(m => m.InquiryModule),
        pathMatch: 'full',
      },
    ]),
  ],
  providers: [
    MyService, // That would one get destroyed if parent route does have `destroyOnNavigationAway: true`
  ],
})
export class MyModule { }

My use case

More info about my project I do have a service, that uses google map (from `@angular/google-maps`), it stores a reference to the map object. I did make all `ngOnDestroy` hooks for cleanup, but if they are never called I am screwed. I also just started facing an issue, that I thought should never happen, as when I navigate outside of the module and back in (via `RouterModule`), non of the init function of the service are run as the service is still alive! ```ts @NgModule({ declarations: [ InquiryComponent, VirtualForDirective, CreateAreasComponent, SelectAreasComponent, RequestAreasComponent, AddRequestedComponent, BlurOnKeysDirective, FocusOnCreationDirective, NarrowAreasComponent, NarrowAreasDialogComponent, ], imports: [ CommonModule, FormsModule, RouterModule.forChild([ { path: '', component: InquiryComponent, pathMatch: 'full', }, ]), GoogleMapsModule, // More material-angular modules // My common modules ], providers: [ InquiryService, // These are not getting destroyed SelectingService, ], }) export class InquiryModule { } ```
Akxe commented 2 years ago

I was thinking about the theme meantime and I am still not sure how the full picture can be analyzed by relatively simple rules to have the clear decision if it's or it isn't possible to destroy the module instance. It's not possible to take only some variants, it's necessary to see all possibilities.

Why would angular should not know if a module is being used? When navigating away angular checks how many are using routers are using given module and act accordingly. It looks pretty easy to me. Even easier as most app use only one router...

angular-robot[bot] commented 2 years ago

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

vazsonyidl commented 2 years ago

I am not sure how many developers know - if you have a huge application -, hence no modules have been ever destroyed, all services that had been provided on any module with providers: [HERE] never got destroyed - as of my current POC and experience.

So: if you are navigating all over your application, at the end, you find yourself there are many services and modules kept in memory.

Please correct me if I am wrong in this.

Ps: There is a partial solution for this, if you provide your service on component based, with providers on the Component decorator, your service actually get destroyed. But this also not helps wit the problem that modules got never destroyed.

Any suggestion on these in the near future?

Akxe commented 2 years ago

@vazsonyidl if you want this behaviour to change, you have to vote thumbs up on the bot answer. Once there are more than 20 of them this will be resolved.


Added info about the current situation in post scriptum.

Your assumptions are correct according to my tests of mine. Lazy loaded modules are never destroyed. For me, this will cause a lot of problems as I optimize for speed over memory, but doing so for every route will result in too much memory consumption.

As for the current solution, someone from the Angular team said, that it could be destroyed after CanDeactivate or a similar event of navigation.

mlc-mlapis commented 2 years ago

There is the destroy() method that can be used for it.

abstract class NgModuleRef<T> {
  abstract injector: Injector
  abstract componentFactoryResolver: ComponentFactoryResolver
  abstract instance: T
  abstract destroy(): void
  abstract onDestroy(callback: () => void): void
}
Akxe commented 2 years ago

@mlc-mlapis The hard part is getting hold of the module at the right time generically.

  1. How do you get hold of the module that is being navigated away?
  2. How do you know the given module is used only by the route that is being navigated away?
  3. Maybe other important questions
mlc-mlapis commented 2 years ago

@Akxe Yes, I have the same questions, as I've commented above already.

You must consider that a module can be loaded and instantiated programmatically (when you get NgModuleRef directly) fully without using a router. But generally, you have to manage it by your code.

Otherwise, you can inject private _ngModule: NgModuleRef<AnyModule> in a constructor to get NgModuleRef of your module. But again, the rest of the logic you must solve by your code. Probably using a dictionary, registering instantiated modules, and on some condition, destroying them.

angular-robot[bot] commented 2 years ago

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

Rush commented 2 years ago

Well. After using Angular for 2 years I finally have a use case for using lazy modules .. only to discover they don't get unloaded when navigating away from a lazy loaded path. A pity.

pawelczak commented 2 years ago

Can the subject be reviewed again, community vote process gathered 25 votes.

pavangayakwad commented 2 years ago

Not destroying a module when navigating away from it is a memory inefficient approach. I don't think this has to be the feature request looking for maximum vote. It is a needed fix with the best interest of offering optimized solution. Voting should never be an option for this kind of request/bug.

harikvpy commented 2 years ago

+1 for this feature

Akxe commented 2 years ago

@petebacondarwin What can we do to move this feature/fix further along?

petebacondarwin commented 2 years ago

@Akxe - I think that your particular use case is when navigating away from a route, right? So this is a feature request for the router - compared to some general module destruction feature.

I would like to get the view of @atscott on this. I think the problem is that I believe the router caches and reuses ModuleRef's for various reasons, and we would need to be sure that we are not breaking things by destroying them.

I have added it to today's framework meeting agenda for discussion.

mlc-mlapis commented 2 years ago

@petebacondarwin There's a relation to how to get NgModuleRefs, even for modules instantiated by the router, and be able to call their .destroy() methods at all. Maybe also by a module id?

petebacondarwin commented 2 years ago

I believe that the router holds references to the NgModuleRefs that it creates so it should be able to destroy them safely. The module id can only really be used to get the "type" of an NgModule class, not an instance of one (a.k.a. an NgModuleRef).

mlc-mlapis commented 2 years ago

@petebacondarwin I suppose that those NgModuleRefs are cached somewhere by the router, but how to get them if we think about the possibility to use the already existed method .destroy() and manage the lifecycle programmatically?

petebacondarwin commented 2 years ago

Exactly! That is why I think this is a feature request for the router: to find a way either for it to expose those refs so that they can be destroyed, or to manage the destruction of them itself - for example by a route config property such as destroyOnNavigationAway: true see https://github.com/angular/angular/issues/37095#issuecomment-833566653

kewde commented 2 years ago

I have already tried to upstream an improvement for destroying modules, feel free to take inspiration or the code: https://github.com/angular/angular/pull/28405

yliu7810 commented 2 years ago

I came from the React world, it's much easier to cancel a request with axios, Angular is just over-complicated and don't work. I would assume the solution of this simple problem should've already be baked in the Injectable class. I have wasted lots of time to debug it until I've found this thread, what a disappointment.

Rush commented 2 years ago

Canceling requests is not a problem. Angular's HttpClient is based on RxJS. You can unsubscribe from an active request which cancels it

yliu7810 commented 2 years ago

Canceling requests is not a problem. Angular's HttpClient is based on RxJS. You can unsubscribe from an active request which cancels it

I written a centralize http request service to handle the data parsing and subscription, the unsubscription is getting called by the ngOnDestroy method. I expect the components which use this service will trigger this method but it doesn't. It would be anti-pattern to manually called unsubscribe on every ngOnDestroy in the every components that use this service.

Rush commented 2 years ago

@yliu7810 I agree that lifecycle events should be exposed as observables by default. I have been using https://www.npmjs.com/package/@ngneat/until-destroy - it solves the problem for good

Harpush commented 1 year ago

Any news concerning this? Right now you can't use probidedIn: ComponentClass but can use it on a module. So you have to decide - either provide on a component and you get automatic destroy but you don't get lazy load where used (if lazy load scope and usage scope are different) or provide in a module which allows correct lazy load but no automatic destroy. With the new angular 14 route providers - another option is allowing two arrays - persisting providers vs destroyed on route change providers to allow maximum flexibility.

7Mica commented 1 year ago

I can't belive this still a thing on Angular 15 and the upcoming 16. If someone gets trouble with this, it will be a nightmere to find out what is happening.

matthiasweiss commented 1 year ago

+1

Iamthereality commented 9 months ago

Is there any news about implementing module destruction?

holger-at-schottel-de commented 8 months ago

+1

hvma411 commented 7 months ago

Three years since this issue was created and still nothing? 🙄

k290 commented 7 months ago

This would be good to have

Iamthereality commented 4 months ago

It is more likely the Angular team won't implement module destruction due to changing main development vector to the standalone components which are the replacement for current approach and perhaps in the future versions they'll deprecate module usage.

Harpush commented 4 months ago

It is more likely the Angular team won't implement module destruction due to changing main development vector to the standalone components which are the replacement for current approach and perhaps in the future versions they'll deprecate module usage.

Router providers destruction can still be implemented

Harpush commented 1 week ago

I created a semi solution to transient route providers with the added benefit of allowing the same lazy load benefits we have with provided in root but for route providers. The main idea is allowing to inject a service from a transient route injector (destroyed on navigating away) and without explicitly providing the service in the route (app providers vs provided in root as an example). The first part is defining the transient provider:

// A token to identify the transient providers manager for this route
const TRANSIENT_HOME_PROVIDERS_MANAGER =
  createTransientRouteProvidersManagerToken();

// The transient inject function for this route
export const injectHomeTransient = createInjectRouteTransientProviderFn(
  TRANSIENT_HOME_PROVIDERS_MANAGER
);

export const HOME_ROUTES: Route[] = [
  {
    path: '',
    component: HomeComponent,
    children: [
      {
        path: 'test1',
        loadComponent: () =>
          import('./test1.component').then((mod) => mod.Test1Component),
      },
      {
        path: 'test2',
        loadComponent: () =>
          import('./test2.component').then((mod) => mod.Test2Component),
      },
    ],
    providers: [
      // Any provider or component on this route or below can use injectHomeTransient
      provideTransientRouteProvidersManager(TRANSIENT_HOME_PROVIDERS_MANAGER)
    ],
  },
];

The second part is using the injectHomeTransient function in Test2Component for example:

injectHomeTransient(TestService)

What will actually happen is:

  1. Until Test2Component is navigated to - TestService won't be created
  2. TestService will be lazy loaded at the same chunk as Test2Component
  3. When navigating between test1 and test2 routes - TestService won't get destroyed
  4. When navigating away from HOME_ROUTES - TestService will get destroyed.

A few important points:

  1. Regular (singleton) services provided on the route or further below shouldn't inject transient providers as it can cause memory leaks (unless used with runInInjectionContext in a function without storing it as a property).
  2. RouteReuseStrategy will keep components and won't destroy them which conflicts with transient providers (similar to point 1).

Those two points above can be solved by either keeping the transient provider even after navigating away (defeats the purpose though) or identifying those cases and throwing an error or just avoiding it without protections.

Currently those protections are missing from the stackblitz and not all inject function overrides are implemented for the new custom inject function.

Link to stackblitz: https://stackblitz.com/edit/stackblitz-starters-zs2ahz