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

Dyn cmp destroy #1372

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -33,6 +33,10 @@ export class AbstractChangeDetector extends ChangeDetector {
cd.parent = this;
}

removeShadowDomChild(cd:ChangeDetector) {
ListWrapper.remove(this.shadowDomChildren, cd);
}

remove() {
this.parent.removeChild(this);
}
Expand Down
1 change: 1 addition & 0 deletions modules/angular2/src/change_detection/interfaces.js
Expand Up @@ -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() {}
Expand Down
Expand Up @@ -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
}

Expand All @@ -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
Expand Down
10 changes: 9 additions & 1 deletion 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';
Expand Down Expand Up @@ -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);
}
}
27 changes: 20 additions & 7 deletions modules/angular2/src/core/compiler/view.js
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand Down Expand Up @@ -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}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find tests checking the two assertions. Could you add some?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

}
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
Expand Down
8 changes: 4 additions & 4 deletions modules/angular2/src/core/compiler/view_factory.js
Expand Up @@ -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];
Expand All @@ -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
Expand Down
9 changes: 9 additions & 0 deletions 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';
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 5 additions & 0 deletions modules/angular2/src/render/dom/view/view.js
Expand Up @@ -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;
}
}
}

Expand Down
3 changes: 1 addition & 2 deletions modules/angular2/src/render/dom/view/view_factory.js
Expand Up @@ -84,7 +84,6 @@ export class ViewFactory {
} else {
viewRootNodes = [rootElementClone];
}

var binders = protoView.elementBinders;
var boundTextNodes = [];
var boundElements = ListWrapper.createFixedSize(binders.length);
Expand Down Expand Up @@ -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);
}
Expand Down
9 changes: 8 additions & 1 deletion 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';
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 12 additions & 0 deletions modules/angular2/src/test_lib/test_lib.es6
Expand Up @@ -272,6 +272,16 @@ _global.beforeEach(function() {
});

export class SpyObject {
constructor(type = null) {
if (type) {
for (var prop in type.prototype) {
var m = type.prototype[prop];
if (typeof m === 'function') {
this.spy(prop);
}
}
}
}
spy(name){
if (! this[name]) {
this[name] = this._createGuinnessCompatibleSpy();
Expand All @@ -286,6 +296,8 @@ export class SpyObject {
_createGuinnessCompatibleSpy(){
var newSpy = jasmine.createSpy();
newSpy.andCallFake = newSpy.and.callFake;
// return null by default to satisfy our rtts asserts
newSpy.and.returnValue(null);
return newSpy;
}
}
Expand Down
Expand Up @@ -484,6 +484,13 @@ export function main() {

expect(parent.lightDomChildren).toEqual([]);
});

it("should remove shadow dom children", () => {
parent.addShadowDomChild(child);
parent.removeShadowDomChild(child);

expect(parent.shadowDomChildren.length).toEqual(0);
});
});
});

Expand Down
105 changes: 95 additions & 10 deletions 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, []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to do it better, but this is a bit ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ElementRef has functions instead of property getters I could mock them. Do you know a way to do this for a getter?

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', () => {
Expand All @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Log from utils.js:

childView.spy('hydrate').andCallFake(log.fn("hydrate"));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work in Dart checked mode, as the function returned by log.fn takes no argument...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can change it to be as follows:

fn(value) {
    return (a=null, b=null, c=null, d=null, e=null) => {
      ListWrapper.push(this._result, value);
    }
  }

Then it will work.

});
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)}}