fix(ivy): ngOnDestroy hooks should be called on providers (#27955)

PR Close #27955

packages/core/src/render3/di.ts

@@ -503,6 +503,10 @@ export function getNodeInjectable(
     setTNodeAndViewData(tNode, lData);
     try {
       value = lData[index] = factory.factory(null, tData, lData, tNode);
+      const tView = lData[TVIEW];
+      if (value && factory.isProvider && value.ngOnDestroy) {
+        (tView.destroyHooks || (tView.destroyHooks = [])).push(index, value.ngOnDestroy);
+      }
     } finally {
       if (factory.injectImpl) setInjectImplementation(previousInjectImplementation);
       setIncludeViewProviders(previousIncludeViewProviders);

packages/core/src/render3/di_setup.ts

@@ -83,7 +83,8 @@ function resolveProvider(
 
     if (isTypeProvider(provider) || !provider.multi) {
       // Single provider case: the factory is created and pushed immediately
-      const factory = new NodeInjectorFactory(providerFactory, isViewProvider, directiveInject);
+      const factory =
+          new NodeInjectorFactory(providerFactory, isViewProvider, true, directiveInject);
       const existingFactoryIndex = indexOf(
           token, tInjectables, isViewProvider ? beginIndex : beginIndex + cptViewProvidersCount,
           endIndex);
@@ -245,7 +246,7 @@ function multiFactory(
         this: NodeInjectorFactory, _: null, tData: TData, lData: LView, tNode: TElementNode) => any,
     index: number, isViewProvider: boolean, isComponent: boolean,
     f: () => any): NodeInjectorFactory {
-  const factory = new NodeInjectorFactory(factoryFn, isViewProvider, directiveInject);
+  const factory = new NodeInjectorFactory(factoryFn, isViewProvider, true, directiveInject);
   factory.multi = [];
   factory.index = index;
   factory.componentProviders = 0;

packages/core/src/render3/instructions.ts

@@ -1732,7 +1732,8 @@ function baseResolveDirective<T>(
     tView: TView, viewData: LView, def: DirectiveDef<T>,
     directiveFactory: (t: Type<T>| null) => any) {
   tView.data.push(def);
-  const nodeInjectorFactory = new NodeInjectorFactory(directiveFactory, isComponentDef(def), null);
+  const nodeInjectorFactory =
+      new NodeInjectorFactory(directiveFactory, isComponentDef(def), false, null);
   tView.blueprint.push(nodeInjectorFactory);
   viewData.push(nodeInjectorFactory);
 }

packages/core/src/render3/interfaces/injector.ts

@@ -234,6 +234,10 @@ export class NodeInjectorFactory {
        * Set to `true` if the token is declared in `viewProviders` (or if it is component).
        */
       isViewProvider: boolean,
+      /**
+       * Set to `true` if the token is a provider, and not a directive.
+       */
+      public isProvider: boolean,
       injectImplementation: null|(<T>(token: Type<T>|InjectionToken<T>, flags: InjectFlags) => T)) {
     this.canSeeViewProviders = isViewProvider;
     this.injectImpl = injectImplementation;

packages/core/test/linker/change_detection_integration_spec.ts

@@ -1167,23 +1167,22 @@ const TEST_COMPILER_PROVIDERS: Provider[] = [
              ]);
            }));
 
-        fixmeIvy('FW-848: ngOnDestroy hooks are not called on providers')
+        it('should call ngOnDestroy on an injectable class', fakeAsync(() => {
-            .it('should call ngOnDestroy on an injectable class', fakeAsync(() => {
+             TestBed.overrideDirective(
-                  TestBed.overrideDirective(
+                 TestDirective, {set: {providers: [InjectableWithLifecycle]}});
-                      TestDirective, {set: {providers: [InjectableWithLifecycle]}});
 
-                  const ctx = createCompFixture('<div testDirective="dir"></div>', TestComponent);
+             const ctx = createCompFixture('<div testDirective="dir"></div>', TestComponent);
 
-                  ctx.debugElement.children[0].injector.get(InjectableWithLifecycle);
+             ctx.debugElement.children[0].injector.get(InjectableWithLifecycle);
-                  ctx.detectChanges(false);
+             ctx.detectChanges(false);
 
-                  ctx.destroy();
+             ctx.destroy();
 
-                  // We don't care about the exact order in this test.
+             // We don't care about the exact order in this test.
-                  expect(directiveLog.filter(['ngOnDestroy']).sort()).toEqual([
+             expect(directiveLog.filter(['ngOnDestroy']).sort()).toEqual([
-                    'dir.ngOnDestroy', 'injectable.ngOnDestroy'
+               'dir.ngOnDestroy', 'injectable.ngOnDestroy'
-                  ]);
+             ]);
-                }));
+           }));
       });
     });
 

packages/core/test/linker/view_injector_integration_spec.ts

@@ -465,38 +465,37 @@ class TestComp {
            expect(comp.componentInstance.a).toBe('aValue');
          });
 
-      fixmeIvy('FW-848: ngOnDestroy hooks are not called on providers')
+      it('should support ngOnDestroy for lazy providers', () => {
-          .it('should support ngOnDestroy for lazy providers', () => {
+        let created = false;
-            let created = false;
+        let destroyed = false;
-            let destroyed = false;
 
-            class SomeInjectable {
+        class SomeInjectable {
-              constructor() { created = true; }
+          constructor() { created = true; }
-              ngOnDestroy() { destroyed = true; }
+          ngOnDestroy() { destroyed = true; }
-            }
+        }
 
-            @Component({providers: [SomeInjectable], template: ''})
+        @Component({providers: [SomeInjectable], template: ''})
-            class SomeComp {
+        class SomeComp {
-            }
+        }
 
-            TestBed.configureTestingModule({declarations: [SomeComp]});
+        TestBed.configureTestingModule({declarations: [SomeComp]});
 
 
-            let compRef = TestBed.createComponent(SomeComp).componentRef;
+        let compRef = TestBed.createComponent(SomeComp).componentRef;
-            expect(created).toBe(false);
+        expect(created).toBe(false);
-            expect(destroyed).toBe(false);
+        expect(destroyed).toBe(false);
 
-            // no error if the provider was not yet created
+        // no error if the provider was not yet created
-            compRef.destroy();
+        compRef.destroy();
-            expect(created).toBe(false);
+        expect(created).toBe(false);
-            expect(destroyed).toBe(false);
+        expect(destroyed).toBe(false);
 
-            compRef = TestBed.createComponent(SomeComp).componentRef;
+        compRef = TestBed.createComponent(SomeComp).componentRef;
-            compRef.injector.get(SomeInjectable);
+        compRef.injector.get(SomeInjectable);
-            expect(created).toBe(true);
+        expect(created).toBe(true);
-            compRef.destroy();
+        compRef.destroy();
-            expect(destroyed).toBe(true);
+        expect(destroyed).toBe(true);
-          });
+      });
 
       it('should instantiate view providers lazily', () => {
         let created = false;

packages/core/test/render3/providers_spec.ts

@@ -1265,6 +1265,96 @@ describe('providers', () => {
       expect(injector.get(Some).location).toEqual('From app component');
     });
   });
+
+  describe('lifecycles', () => {
+    it('should execute ngOnDestroy hooks on providers (and only this one)', () => {
+      const logs: string[] = [];
+
+      @Injectable()
+      class InjectableWithLifeCycleHooks {
+        ngOnChanges() { logs.push('Injectable OnChanges'); }
+        ngOnInit() { logs.push('Injectable OnInit'); }
+        ngDoCheck() { logs.push('Injectable DoCheck'); }
+        ngAfterContentInit() { logs.push('Injectable AfterContentInit'); }
+        ngAfterContentChecked() { logs.push('Injectable AfterContentChecked'); }
+        ngAfterViewInit() { logs.push('Injectable AfterViewInit'); }
+        ngAfterViewChecked() { logs.push('Injectable gAfterViewChecked'); }
+        ngOnDestroy() { logs.push('Injectable OnDestroy'); }
+      }
+
+      @Component({template: `<span></span>`, providers: [InjectableWithLifeCycleHooks]})
+      class MyComponent {
+        constructor(foo: InjectableWithLifeCycleHooks) {}
+
+        static ngComponentDef = defineComponent({
+          type: MyComponent,
+          selectors: [['my-comp']],
+          factory: () => new MyComponent(directiveInject(InjectableWithLifeCycleHooks)),
+          consts: 1,
+          vars: 0,
+          template: (rf: RenderFlags, ctx: MyComponent) => {
+            if (rf & RenderFlags.Create) {
+              element(0, 'span');
+            }
+          },
+          features: [ProvidersFeature([InjectableWithLifeCycleHooks])]
+        });
+      }
+
+      @Component({
+        template: `
+        <div>
+        % if (ctx.condition) {
+          <my-comp></my-comp>
+        % }
+        </div>
+        `,
+      })
+      class App {
+        public condition = true;
+
+        static ngComponentDef = defineComponent({
+          type: App,
+          selectors: [['app-cmp']],
+          factory: () => new App(),
+          consts: 2,
+          vars: 0,
+          template: (rf: RenderFlags, ctx: App) => {
+            if (rf & RenderFlags.Create) {
+              elementStart(0, 'div');
+              { container(1); }
+              elementEnd();
+            }
+            if (rf & RenderFlags.Update) {
+              containerRefreshStart(1);
+              {
+                if (ctx.condition) {
+                  let rf1 = embeddedViewStart(1, 2, 1);
+                  {
+                    if (rf1 & RenderFlags.Create) {
+                      element(0, 'my-comp');
+                    }
+                  }
+                  embeddedViewEnd();
+                }
+              }
+              containerRefreshEnd();
+            }
+          },
+          directives: [MyComponent]
+        });
+      }
+
+      const fixture = new ComponentFixture(App);
+      fixture.update();
+      expect(fixture.html).toEqual('<div><my-comp><span></span></my-comp></div>');
+
+      fixture.component.condition = false;
+      fixture.update();
+      expect(fixture.html).toEqual('<div></div>');
+      expect(logs).toEqual(['Injectable OnDestroy']);
+    });
+  });
 });
 interface ComponentTest {
   providers?: Provider[];