fix(ivy): properly destroy views created by ComponentFactory (angular#27676)

pkozlowski-opensource · mhevery · commit b00aeeff3762 · 2018-12-14T15:22:11.000-08:00
PR Close angular#27676
diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts
@@ -22,11 +22,10 @@ import {assertComponentType, assertDefined} from './assert';
 import {LifecycleHooksFeature, createRootComponent, createRootComponentView, createRootContext} from './component';
 import {getComponentDef} from './definition';
 import {NodeInjector} from './di';
-import {createLView, createNodeAtIndex, createTView, createViewNode, elementCreate, locateHostElement, refreshDescendantViews} from './instructions';
+import {addToViewTree, createLView, createNodeAtIndex, createTView, createViewNode, elementCreate, locateHostElement, refreshDescendantViews} from './instructions';
 import {ComponentDef, RenderFlags} from './interfaces/definition';
 import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeType} from './interfaces/node';
 import {RElement, RendererFactory3, domRendererFactory3, isProceduralRenderer} from './interfaces/renderer';
-import {SanitizerFn} from './interfaces/sanitization';
 import {HEADER_OFFSET, LView, LViewFlags, RootContext, TVIEW} from './interfaces/view';
 import {enterView, leaveView} from './state';
 import {defaultScheduler, getTNode} from './util';
@@ -169,6 +168,7 @@ export class ComponentFactory<T> extends viewEngine_ComponentFactory<T> {
 
       const componentView = createRootComponentView(
           hostRNode, this.componentDef, rootLView, rendererFactory, renderer);
+
       tElementNode = getTNode(0, rootLView) as TElementNode;
 
       // Transform the arrays of native nodes into a structure that can be consumed by the
@@ -207,6 +207,8 @@ export class ComponentFactory<T> extends viewEngine_ComponentFactory<T> {
       component = createRootComponent(
           componentView, this.componentDef, rootLView, rootContext, [LifecycleHooksFeature]);
 
+      addToViewTree(rootLView, HEADER_OFFSET, componentView);
+
       refreshDescendantViews(rootLView, RenderFlags.Create);
     } finally {
       leaveView(oldLView, true);
diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
@@ -455,16 +455,18 @@ export function getParentState(state: LView | LContainer, rootView: LView): LVie
 }
 
 /**
- * Removes all listeners and call all onDestroys in a given view.
+ * Calls onDestroys hooks for all directives and pipes in a given view and then removes all
+ * listeners. Listeners are removed as the last step so events delivered in the onDestroys hooks
+ * can be propagated to @Output listeners.
  *
  * @param view The LView to clean up
  */
 function cleanUpView(viewOrContainer: LView | LContainer): void {
   if ((viewOrContainer as LView).length >= HEADER_OFFSET) {
     const view = viewOrContainer as LView;
-    removeListeners(view);
     executeOnDestroys(view);
     executePipeOnDestroys(view);
+    removeListeners(view);
     const hostTNode = view[HOST_NODE];
     // For component views only, the local renderer is destroyed as clean up time.
     if (hostTNode && hostTNode.type === TNodeType.Element && isProceduralRenderer(view[RENDERER])) {
diff --git a/packages/core/test/linker/change_detection_integration_spec.ts b/packages/core/test/linker/change_detection_integration_spec.ts
@@ -1104,77 +1104,70 @@ const TEST_COMPILER_PROVIDERS: Provider[] = [
       });
 
       describe('ngOnDestroy', () => {
-        fixmeIvy('FW-763: LView tree not properly constructed / destroyed')
-            .it('should be called on view destruction', fakeAsync(() => {
-                  const ctx = createCompFixture('<div testDirective="dir"></div>');
-                  ctx.detectChanges(false);
+        it('should be called on view destruction', fakeAsync(() => {
+             const ctx = createCompFixture('<div testDirective="dir"></div>');
+             ctx.detectChanges(false);
 
-                  ctx.destroy();
+             ctx.destroy();
 
-                  expect(directiveLog.filter(['ngOnDestroy'])).toEqual(['dir.ngOnDestroy']);
-                }));
+             expect(directiveLog.filter(['ngOnDestroy'])).toEqual(['dir.ngOnDestroy']);
+           }));
 
-        fixmeIvy('FW-763: LView tree not properly constructed / destroyed')
-            .it('should be called after processing the content and view children', fakeAsync(() => {
-                  TestBed.overrideComponent(AnotherComponent, {
-                    set: new Component({
-                      selector: 'other-cmp',
-                      template: '<div testDirective="viewChild"></div>'
-                    })
-                  });
+        it('should be called after processing the content and view children', fakeAsync(() => {
+             TestBed.overrideComponent(AnotherComponent, {
+               set: new Component(
+                   {selector: 'other-cmp', template: '<div testDirective="viewChild"></div>'})
+             });
 
-                  const ctx = createCompFixture(
-                      '<div testDirective="parent"><div *ngFor="let x of [0,1]" testDirective="contentChild{{x}}"></div>' +
-                          '<other-cmp></other-cmp></div>',
-                      TestComponent);
+             const ctx = createCompFixture(
+                 '<div testDirective="parent"><div *ngFor="let x of [0,1]" testDirective="contentChild{{x}}"></div>' +
+                     '<other-cmp></other-cmp></div>',
+                 TestComponent);
 
-                  ctx.detectChanges(false);
-                  ctx.destroy();
+             ctx.detectChanges(false);
+             ctx.destroy();
 
-                  expect(directiveLog.filter(['ngOnDestroy'])).toEqual([
-                    'contentChild0.ngOnDestroy', 'contentChild1.ngOnDestroy',
-                    'viewChild.ngOnDestroy', 'parent.ngOnDestroy'
-                  ]);
-                }));
+             expect(directiveLog.filter(['ngOnDestroy'])).toEqual([
+               'contentChild0.ngOnDestroy', 'contentChild1.ngOnDestroy', 'viewChild.ngOnDestroy',
+               'parent.ngOnDestroy'
+             ]);
+           }));
 
-        fixmeIvy('FW-763: LView tree not properly constructed / destroyed')
-            .it('should be called in reverse order so the child is always notified before the parent',
-                fakeAsync(() => {
-                  const ctx = createCompFixture(
-                      '<div testDirective="parent"><div testDirective="child"></div></div><div testDirective="sibling"></div>');
+        it('should be called in reverse order so the child is always notified before the parent',
+           fakeAsync(() => {
+             const ctx = createCompFixture(
+                 '<div testDirective="parent"><div testDirective="child"></div></div><div testDirective="sibling"></div>');
 
-                  ctx.detectChanges(false);
-                  ctx.destroy();
+             ctx.detectChanges(false);
+             ctx.destroy();
 
-                  expect(directiveLog.filter(['ngOnDestroy'])).toEqual([
-                    'child.ngOnDestroy', 'parent.ngOnDestroy', 'sibling.ngOnDestroy'
-                  ]);
-                }));
+             expect(directiveLog.filter(['ngOnDestroy'])).toEqual([
+               'child.ngOnDestroy', 'parent.ngOnDestroy', 'sibling.ngOnDestroy'
+             ]);
+           }));
 
-        fixmeIvy('FW-763: LView tree not properly constructed / destroyed')
-            .it('should deliver synchronous events to parent', fakeAsync(() => {
-                  const ctx =
-                      createCompFixture('<div (destroy)="a=$event" onDestroyDirective></div>');
+        it('should deliver synchronous events to parent', fakeAsync(() => {
+             const ctx = createCompFixture('<div (destroy)="a=$event" onDestroyDirective></div>');
 
-                  ctx.detectChanges(false);
-                  ctx.destroy();
+             ctx.detectChanges(false);
+             ctx.destroy();
 
-                  expect(ctx.componentInstance.a).toEqual('destroyed');
-                }));
+             expect(ctx.componentInstance.a).toEqual('destroyed');
+           }));
 
-        fixmeIvy('FW-763: LView tree not properly constructed / destroyed')
-            .it('should call ngOnDestroy on pipes', fakeAsync(() => {
-                  const ctx = createCompFixture('{{true | pipeWithOnDestroy }}');
 
-                  ctx.detectChanges(false);
-                  ctx.destroy();
+        it('should call ngOnDestroy on pipes', fakeAsync(() => {
+             const ctx = createCompFixture('{{true | pipeWithOnDestroy }}');
 
-                  expect(directiveLog.filter(['ngOnDestroy'])).toEqual([
-                    'pipeWithOnDestroy.ngOnDestroy'
-                  ]);
-                }));
+             ctx.detectChanges(false);
+             ctx.destroy();
+
+             expect(directiveLog.filter(['ngOnDestroy'])).toEqual([
+               'pipeWithOnDestroy.ngOnDestroy'
+             ]);
+           }));
 
-        fixmeIvy('FW-763: LView tree not properly constructed / destroyed')
+        fixmeIvy('FW-848: ngOnDestroy hooks are not called on providers')
             .it('should call ngOnDestroy on an injectable class', fakeAsync(() => {
                   TestBed.overrideDirective(
                       TestDirective, {set: {providers: [InjectableWithLifecycle]}});
diff --git a/packages/core/test/linker/integration_spec.ts b/packages/core/test/linker/integration_spec.ts
@@ -768,37 +768,35 @@ function declareTests(config?: {useJit: boolean}) {
         expect(childComponent.myHost).toBeAnInstanceOf(SomeDirective);
       });
 
-      fixmeIvy(
-          'FW-763: LView tree not properly constructed / destroyed for dynamically inserted components')
-          .it('should support events via EventEmitter on regular elements', async(() => {
-                TestBed.configureTestingModule(
-                    {declarations: [MyComp, DirectiveEmittingEvent, DirectiveListeningEvent]});
-                const template = '<div emitter listener></div>';
-                TestBed.overrideComponent(MyComp, {set: {template}});
-                const fixture = TestBed.createComponent(MyComp);
-
-                const tc = fixture.debugElement.children[0];
-                const emitter = tc.injector.get(DirectiveEmittingEvent);
-                const listener = tc.injector.get(DirectiveListeningEvent);
-
-                expect(listener.msg).toEqual('');
-                let eventCount = 0;
+      it('should support events via EventEmitter on regular elements', async(() => {
+           TestBed.configureTestingModule(
+               {declarations: [MyComp, DirectiveEmittingEvent, DirectiveListeningEvent]});
+           const template = '<div emitter listener></div>';
+           TestBed.overrideComponent(MyComp, {set: {template}});
+           const fixture = TestBed.createComponent(MyComp);
 
-                emitter.event.subscribe({
-                  next: () => {
-                    eventCount++;
-                    if (eventCount === 1) {
-                      expect(listener.msg).toEqual('fired !');
-                      fixture.destroy();
-                      emitter.fireEvent('fired again !');
-                    } else {
-                      expect(listener.msg).toEqual('fired !');
-                    }
-                  }
-                });
+           const tc = fixture.debugElement.children[0];
+           const emitter = tc.injector.get(DirectiveEmittingEvent);
+           const listener = tc.injector.get(DirectiveListeningEvent);
+
+           expect(listener.msg).toEqual('');
+           let eventCount = 0;
+
+           emitter.event.subscribe({
+             next: () => {
+               eventCount++;
+               if (eventCount === 1) {
+                 expect(listener.msg).toEqual('fired !');
+                 fixture.destroy();
+                 emitter.fireEvent('fired again !');
+               } else {
+                 expect(listener.msg).toEqual('fired !');
+               }
+             }
+           });
 
-                emitter.fireEvent('fired !');
-              }));
+           emitter.fireEvent('fired !');
+         }));
 
       fixmeIvy(
           'FW-665: Discovery util fails with Unable to find the given context data for the given target')
diff --git a/packages/core/test/linker/query_integration_spec.ts b/packages/core/test/linker/query_integration_spec.ts
@@ -616,9 +616,8 @@ describe('Query API', () => {
           expect(q.query.map((d: TextDirective) => d.text)).toEqual(['1', '2']);
         });
 
-    fixmeIvy(
-        'FW-763 - LView tree not properly constructed / destroyed for dynamically inserted components')
-        .it('should remove manually projected templates if their parent view is destroyed', () => {
+    fixmeIvy('unknown').it(
+        'should remove manually projected templates if their parent view is destroyed', () => {
           const template = `
           <manual-projecting #q><ng-template #tpl><div text="1"></div></ng-template></manual-projecting>
           <div *ngIf="shouldShow">
