fix(view): remove dynamic components when the parent view is dehydrated

Also adds a bunch of unit tests for affected parts.

Fixes angular#1201

Author: tbosch

modules/angular2/src/change_detection/interfaces.js

@@ -49,6 +49,7 @@ export class ChangeDetector {
   addChild(cd:ChangeDetector) {}
   addShadowDomChild(cd:ChangeDetector) {}
   removeChild(cd:ChangeDetector) {}
+  removeShadowDomChild(cd:ChangeDetector) {}
   remove() {}
   hydrate(context:any, locals:Locals, directives:any) {}
   dehydrate() {}

modules/angular2/src/core/compiler/dynamic_component_loader.js

@@ -43,14 +43,12 @@ export class ComponentRef {
 export class DynamicComponentLoader {
   _compiler:Compiler;
   _viewFactory:ViewFactory;
-  _renderer:Renderer;
   _directiveMetadataReader:DirectiveMetadataReader;
 
   constructor(compiler:Compiler, directiveMetadataReader:DirectiveMetadataReader,
               renderer:Renderer, viewFactory:ViewFactory) {
     this._compiler = compiler;
     this._directiveMetadataReader = directiveMetadataReader;
-    this._renderer = renderer;
     this._viewFactory = viewFactory
   }
 
@@ -67,16 +65,13 @@ export class DynamicComponentLoader {
 
     var hostEi = location.elementInjector;
     var hostView = location.hostView;
-
     return this._compiler.compile(type).then(componentProtoView => {
       var component = hostEi.dynamicallyCreateComponent(type, directiveMetadata.annotation, inj);
       var componentView = this._instantiateAndHydrateView(componentProtoView, injector, hostEi, component);
 
       //TODO(vsavkin): do not use component child views as we need to clear the dynamically created views
       //same problem exists on the render side
-      hostView.addComponentChildView(componentView);
-
-      this._renderer.setDynamicComponentView(hostView.render, location.boundElementIndex, componentView.render);
+      hostView.setDynamicComponentChildView(location.boundElementIndex, componentView);
 
       // TODO(vsavkin): return a component ref that dehydrates the component view and removes it
       // from the component child views

modules/angular2/src/core/compiler/element_binder.js

@@ -1,4 +1,4 @@
-import {int, isBlank, BaseException} from 'angular2/src/facade/lang';
+import {int, isBlank, isPresent, BaseException} from 'angular2/src/facade/lang';
 import * as eiModule from './element_injector';
 import {DirectiveBinding} from './element_injector';
 import {List, StringMap} from 'angular2/src/facade/collection';
@@ -32,4 +32,12 @@ export class ElementBinder {
     // updated later, so we are able to resolve cycles
     this.nestedProtoView = null;
   }
+
+  hasStaticComponent() {
+    return isPresent(this.componentDirective) && isPresent(this.nestedProtoView);
+  }
+
+  hasDynamicComponent() {
+    return isPresent(this.componentDirective) && isBlank(this.nestedProtoView);
+  }
 }

modules/angular2/src/core/compiler/view.js

@@ -143,7 +143,6 @@ export class AppView {
     }
 
     var binders = this.proto.elementBinders;
-    var componentChildViewIndex = 0;
     for (var i = 0; i < binders.length; ++i) {
       var componentDirective = binders[i].componentDirective;
       var shadowDomAppInjector = null;
@@ -176,16 +175,15 @@ export class AppView {
         }
       }
 
-      if (isPresent(binders[i].nestedProtoView) && isPresent(componentDirective)) {
-        renderComponentIndex = this.componentChildViews[componentChildViewIndex].internalHydrateRecurse(
+      if (binders[i].hasStaticComponent()) {
+        renderComponentIndex = this.componentChildViews[i].internalHydrateRecurse(
           renderComponentViewRefs,
           renderComponentIndex,
           shadowDomAppInjector,
           elementInjector,
           elementInjector.getComponent(),
           null
         );
-        componentChildViewIndex++;
       }
     }
     this._hydrateChangeDetector();
@@ -198,7 +196,15 @@ export class AppView {
 
     // componentChildViews
     for (var i = 0; i < this.componentChildViews.length; i++) {
-      this.componentChildViews[i].internalDehydrateRecurse();
+      var componentView = this.componentChildViews[i];
+      if (isPresent(componentView)) {
+        componentView.internalDehydrateRecurse();
+        var binder = this.proto.elementBinders[i];
+        if (binder.hasDynamicComponent()) {
+          this.componentChildViews[i] = null;
+          this.changeDetector.removeShadowDomChild(componentView.changeDetector);
+        }
+      }
     }
 
     // elementInjectors
@@ -255,9 +261,16 @@ export class AppView {
     return elementInjector.getDirectiveAtIndex(directive.directiveIndex);
   }
 
-  addComponentChildView(view:AppView) {
-    ListWrapper.push(this.componentChildViews, view);
+  setDynamicComponentChildView(boundElementIndex, view:AppView) {
+    if (!this.proto.elementBinders[boundElementIndex].hasDynamicComponent()) {
+      throw new BaseException(`There is no dynamic component directive at element ${boundElementIndex}`);
+    }
+    if (isPresent(this.componentChildViews[boundElementIndex])) {
+      throw new BaseException(`There already is a bound component at element ${boundElementIndex}`);
+    }
+    this.componentChildViews[boundElementIndex] = view;
     this.changeDetector.addShadowDomChild(view.changeDetector);
+    this.proto.renderer.setDynamicComponentView(this.render, boundElementIndex, view.render);
   }
 
   // implementation of EventDispatcher#dispatchEvent

modules/angular2/src/core/compiler/view_factory.js

@@ -57,7 +57,7 @@ export class ViewFactory {
     var rootElementInjectors = [];
     var preBuiltObjects = ListWrapper.createFixedSize(binders.length);
     var viewContainers = ListWrapper.createFixedSize(binders.length);
-    var componentChildViews = [];
+    var componentChildViews = ListWrapper.createFixedSize(binders.length);
 
     for (var binderIdx = 0; binderIdx < binders.length; binderIdx++) {
       var binder = binders[binderIdx];
@@ -78,13 +78,13 @@ export class ViewFactory {
 
       // componentChildViews
       var bindingPropagationConfig = null;
-      if (isPresent(binder.nestedProtoView) && isPresent(binder.componentDirective)) {
+      if (binder.hasStaticComponent()) {
         var childView = this._createView(binder.nestedProtoView);
-        changeDetector.addChild(childView.changeDetector);
+        changeDetector.addShadowDomChild(childView.changeDetector);
 
         bindingPropagationConfig = new BindingPropagationConfig(childView.changeDetector);
 
-        ListWrapper.push(componentChildViews, childView);
+        componentChildViews[binderIdx] = childView;
       }
 
       // viewContainers

modules/angular2/src/render/dom/view/element_binder.js

@@ -1,3 +1,4 @@
+import {isBlank, isPresent} from 'angular2/src/facade/lang';
 import {AST} from 'angular2/change_detection';
 import {SetterFn} from 'angular2/src/reflection/types';
 import {List, ListWrapper} from 'angular2/src/facade/collection';
@@ -38,6 +39,14 @@ export class ElementBinder {
     this.distanceToParent = distanceToParent;
     this.propertySetters = propertySetters;
   }
+
+  hasStaticComponent() {
+    return isPresent(this.componentId) && isPresent(this.nestedProtoView);
+  }
+
+  hasDynamicComponent() {
+    return isPresent(this.componentId) && isBlank(this.nestedProtoView);
+  }
 }
 
 export class Event {

modules/angular2/src/render/dom/view/view.js

@@ -165,6 +165,11 @@ export class RenderView {
       var cv = this.componentChildViews[i];
       if (isPresent(cv)) {
         cv.dehydrate();
+        if (this.proto.elementBinders[i].hasDynamicComponent()) {
+          ViewContainer.removeViewNodes(cv);
+          this.lightDoms[i] = null;
+          this.componentChildViews[i] = null;
+        }
       }
     }
 

modules/angular2/src/render/dom/view/view_factory.js

@@ -84,7 +84,6 @@ export class ViewFactory {
     } else {
       viewRootNodes = [rootElementClone];
     }
-
     var binders = protoView.elementBinders;
     var boundTextNodes = [];
     var boundElements = ListWrapper.createFixedSize(binders.length);
@@ -133,7 +132,7 @@ export class ViewFactory {
       var element = boundElements[binderIdx];
 
       // static child components
-      if (isPresent(binder.componentId) && isPresent(binder.nestedProtoView)) {
+      if (binder.hasStaticComponent()) {
         var childView = this._createView(binder.nestedProtoView);
         view.setComponentView(this._shadowDomStrategy, binderIdx, childView);
       }

modules/angular2/src/test_lib/test_lib.dart

@@ -1,7 +1,7 @@
 library test_lib.test_lib;
 
 import 'package:guinness/guinness.dart' as gns;
-export 'package:guinness/guinness.dart' hide Expect, expect, NotExpect, beforeEach, it, iit, xit;
+export 'package:guinness/guinness.dart' hide Expect, expect, NotExpect, beforeEach, it, iit, xit, SpyObject;
 import 'package:unittest/unittest.dart' hide expect;
 
 import 'dart:async';
@@ -149,6 +149,13 @@ xit(name, fn) {
   _it(gns.xit, name, fn);
 }
 
+class SpyObject extends gns.SpyObject {
+  // Need to take an optional type as this is required by
+  // the JS SpyObject.
+  SpyObject([type = null]) {
+  }
+}
+
 String elementText(n) {
   hasNodes(n) {
     var children = DOM.childNodes(n);

modules/angular2/test/core/compiler/dynamic_component_loader_spec.js

@@ -1,22 +1,59 @@
-import {ddescribe, describe, it, iit, expect, beforeEach} from 'angular2/test_lib';
+import {
+  AsyncTestCompleter,
+  beforeEach,
+  ddescribe,
+  xdescribe,
+  describe,
+  el,
+  dispatchEvent,
+  expect,
+  iit,
+  inject,
+  beforeEachBindings,
+  it,
+  xit,
+  SpyObject, proxy
+} from 'angular2/test_lib';
+import {IMPLEMENTS} from 'angular2/src/facade/lang';
+import {MapWrapper, ListWrapper} from 'angular2/src/facade/collection';
+import {Promise, PromiseWrapper} from 'angular2/src/facade/async';
 import {DirectiveMetadataReader} from 'angular2/src/core/compiler/directive_metadata_reader';
 import {DynamicComponentLoader} from 'angular2/src/core/compiler/dynamic_component_loader';
-import {Decorator, Viewport} from 'angular2/src/core/annotations/annotations';
-
-@Decorator({selector: 'someDecorator'})
-class SomeDecorator {}
-
-@Viewport({selector: 'someViewport'})
-class SomeViewport {}
+import {Decorator, Viewport, Component} from 'angular2/src/core/annotations/annotations';
+import {ElementRef, ElementInjector, ProtoElementInjector, PreBuiltObjects} from 'angular2/src/core/compiler/element_injector';
+import {Compiler} from 'angular2/src/core/compiler/compiler';
+import {AppProtoView, AppView} from 'angular2/src/core/compiler/view';
+import {ViewFactory} from 'angular2/src/core/compiler/view_factory'
+import {Renderer} from 'angular2/src/render/api';
 
 export function main() {
   describe("DynamicComponentLoader", () => {
+    var compiler;
+    var viewFactory;
+    var directiveMetadataReader;
+    var renderer;
     var loader;
 
-    beforeEach(() => {
-      loader = new DynamicComponentLoader(null, new DirectiveMetadataReader(), null, null);
+    beforeEach( () => {
+      compiler = new SpyCompiler();
+      viewFactory = new SpyViewFactory();
+      renderer = new SpyRenderer();
+      directiveMetadataReader = new DirectiveMetadataReader();
+      loader = new DynamicComponentLoader(compiler, directiveMetadataReader, renderer, viewFactory);;
     });
 
+    function createProtoView() {
+      return new AppProtoView(null, null, null);
+    }
+
+    function createElementRef(view, boundElementIndex) {
+      var peli = new ProtoElementInjector(null, boundElementIndex, []);
+      var eli = new ElementInjector(peli, null);
+      var preBuiltObjects = new PreBuiltObjects(view, null, null, null);
+      eli.instantiateDirectives(null, null, null, preBuiltObjects);
+      return new ElementRef(eli);
+    }
+
     describe("loadIntoExistingLocation", () => {
       describe('Load errors', () => {
         it('should throw when trying to load a decorator', () => {
@@ -29,7 +66,55 @@ export function main() {
             .toThrowError("Could not load 'SomeViewport' because it is not a component.");
         });
       });
+
+      it('should add the child view into the host view', inject([AsyncTestCompleter], (async) => {
+        var log = [];
+        var hostView = new SpyAppView();
+        var childView = new SpyAppView();
+        hostView.spy('setDynamicComponentChildView').andCallFake( (boundElementIndex, childView) => {
+          ListWrapper.push(log, ['setDynamicComponentChildView', boundElementIndex, childView]);
+        });
+        childView.spy('hydrate').andCallFake( (appInjector, hostElementInjector, context, locals) => {
+          ListWrapper.push(log, 'hydrate');
+        });
+        compiler.spy('compile').andCallFake( (_) => PromiseWrapper.resolve(createProtoView()));
+        viewFactory.spy('getView').andCallFake( (_) => childView);
+
+        var elementRef = createElementRef(hostView, 23);
+        loader.loadIntoExistingLocation(SomeComponent, elementRef).then( (componentRef) => {
+          expect(log[0]).toEqual('hydrate');
+          expect(log[1]).toEqual(['setDynamicComponentChildView', 23, childView]);
+          async.done();
+        });
+      }));
+
     });
 
   });
 }
+
+@Decorator({selector: 'someDecorator'})
+class SomeDecorator {}
+
+@Viewport({selector: 'someViewport'})
+class SomeViewport {}
+
+@Component({selector: 'someComponent'})
+class SomeComponent {}
+
+
+@proxy
+@IMPLEMENTS(Compiler)
+class SpyCompiler extends SpyObject {noSuchMethod(m){return super.noSuchMethod(m)}}
+
+@proxy
+@IMPLEMENTS(ViewFactory)
+class SpyViewFactory extends SpyObject {noSuchMethod(m){return super.noSuchMethod(m)}}
+
+@proxy
+@IMPLEMENTS(Renderer)
+class SpyRenderer extends SpyObject {noSuchMethod(m){return super.noSuchMethod(m)}}
+
+@proxy
+@IMPLEMENTS(AppView)
+class SpyAppView extends SpyObject {noSuchMethod(m){return super.noSuchMethod(m)}}