Skip to content

Commit

Permalink
fix(ivy): avoid destroy renderer method invocation for child views (#…
Browse files Browse the repository at this point in the history 
…27592)

Since Renderer is shared across root and child views, we need to avoid `destroy` method invocation for child views and only invoke is for root view when needed. Prior to this change, the `destroy` function was called whenever child view was destroyed, thus causing errors at runtime.

PR Close #27592
  • Loading branch information
@AndrewKushnir @mhevery
AndrewKushnir authored and mhevery committed Dec 12, 2018
1 parent 9c7fb0d commit 37c05bd
Show file tree
Hide file tree
Showing 5 changed files with 194 additions and 129 deletions.
3 changes: 2 additions & 1 deletion packages/core/src/render3/node_manipulation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,9 @@ function cleanUpView(viewOrContainer: LView | LContainer): void {
removeListeners(view);
executeOnDestroys(view);
executePipeOnDestroys(view);
const hostTNode = view[HOST_NODE];
// For component views only, the local renderer is destroyed as clean up time.
if (view[TVIEW].id === -1 && isProceduralRenderer(view[RENDERER])) {
if (hostTNode && hostTNode.type === TNodeType.Element && isProceduralRenderer(view[RENDERER])) {
ngDevMode && ngDevMode.rendererDestroy++;
(view[RENDERER] as ProceduralRenderer3).destroy();
}
Expand Down
61 changes: 60 additions & 1 deletion packages/core/test/render3/component_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {ComponentDef, RenderFlags} from '../../src/render3/interfaces/definition

import {NgIf} from './common_with_def';
import {getRendererFactory2} from './imported_renderer2';
import {ComponentFixture, containerEl, createComponent, renderComponent, renderToHtml, requestAnimationFrame, toHtml} from './render_util';
import {ComponentFixture, MockRendererFactory, containerEl, createComponent, renderComponent, renderToHtml, requestAnimationFrame, toHtml} from './render_util';

describe('component', () => {
class CounterComponent {
Expand Down Expand Up @@ -152,6 +152,65 @@ describe('component', () => {

});

it('should not invoke renderer destroy method for embedded views', () => {
let comp: Comp;

function MyComponent_div_Template_2(rf: any, ctx: any) {
if (rf & RenderFlags.Create) {
elementStart(0, 'div');
text(1, 'Child view');
elementEnd();
}
}

class Comp {
visible = true;

static ngComponentDef = defineComponent({
type: Comp,
selectors: [['comp']],
consts: 3,
vars: 1,
factory: () => {
comp = new Comp();
return comp;
},
directives: [NgIf],
/**
* <div>Root view</div>
* <div *ngIf="visible">Child view</div>
*/
template: function(rf: RenderFlags, ctx: Comp) {
if (rf & RenderFlags.Create) {
elementStart(0, 'div');
text(1, 'Root view');
elementEnd();
template(2, MyComponent_div_Template_2, 2, 0, null, [1, 'ngIf']);
}
if (rf & RenderFlags.Update) {
elementProperty(2, 'ngIf', bind(ctx.visible));
}
}
});
}

const rendererFactory = new MockRendererFactory(['destroy']);
const fixture = new ComponentFixture(Comp, {rendererFactory});

comp !.visible = false;
fixture.update();

comp !.visible = true;
fixture.update();

const renderer = rendererFactory.lastRenderer !;
const destroySpy = renderer.spies['destroy'];

// we should never see `destroy` method being called
// in case child views are created/removed
expect(destroySpy.calls.count()).toBe(0);
});

describe('component with a container', () => {

function showItems(rf: RenderFlags, ctx: {items: string[]}) {
Expand Down
62 changes: 4 additions & 58 deletions packages/core/test/render3/integration_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,20 @@
*/

import {ElementRef, TemplateRef, ViewContainerRef} from '@angular/core';
import {RendererStyleFlags2, RendererType2} from '../../src/render/api';
import {RendererType2} from '../../src/render/api';
import {AttributeMarker, defineComponent, defineDirective, templateRefExtractor} from '../../src/render3/index';

import {allocHostVars, bind, container, containerRefreshEnd, containerRefreshStart, element, elementAttribute, elementClassProp, elementContainerEnd, elementContainerStart, elementEnd, elementProperty, elementStart, elementStyleProp, elementStyling, elementStylingApply, embeddedViewEnd, embeddedViewStart, interpolation1, interpolation2, interpolation3, interpolation4, interpolation5, interpolation6, interpolation7, interpolation8, interpolationV, load, projection, projectionDef, reference, text, textBinding, template, elementStylingMap, directiveInject} from '../../src/render3/instructions';
import {InitialStylingFlags, RenderFlags} from '../../src/render3/interfaces/definition';
import {RElement, Renderer3, RendererFactory3, domRendererFactory3, RText, RComment, RNode, RendererStyleFlags3, ProceduralRenderer3} from '../../src/render3/interfaces/renderer';
import {RElement, Renderer3, RendererFactory3, domRendererFactory3} from '../../src/render3/interfaces/renderer';
import {NO_CHANGE} from '../../src/render3/tokens';
import {HEADER_OFFSET, CONTEXT} from '../../src/render3/interfaces/view';
import {enableBindings, disableBindings} from '../../src/render3/state';
import {sanitizeUrl} from '../../src/sanitization/sanitization';
import {Sanitizer, SecurityContext} from '../../src/sanitization/security';

import {NgIf} from './common_with_def';
import {ComponentFixture, TemplateFixture, createComponent, renderToHtml} from './render_util';
import {ComponentFixture, MockRendererFactory, TemplateFixture, createComponent, renderToHtml} from './render_util';
import {getLContext} from '../../src/render3/context_discovery';
import {StylingIndex} from '../../src/render3/interfaces/styling';
import {MONKEY_PATCH_KEY_NAME} from '../../src/render3/interfaces/context';
Expand Down Expand Up @@ -2652,58 +2652,4 @@ class ProxyRenderer3Factory implements RendererFactory3 {
this.lastCapturedType = rendererType;
return domRendererFactory3.createRenderer(hostElement, rendererType);
}
}

class MockRendererFactory implements RendererFactory3 {
lastRenderer: any;
private _spyOnMethods: string[];

constructor(spyOnMethods?: string[]) { this._spyOnMethods = spyOnMethods || []; }

createRenderer(hostElement: RElement|null, rendererType: RendererType2|null): Renderer3 {
const renderer = this.lastRenderer = new MockRenderer(this._spyOnMethods);
return renderer;
}
}

class MockRenderer implements ProceduralRenderer3 {
public spies: {[methodName: string]: any} = {};

constructor(spyOnMethods: string[]) {
spyOnMethods.forEach(methodName => {
this.spies[methodName] = spyOn(this as any, methodName).and.callThrough();
});
}

destroy(): void {}
createComment(value: string): RComment { return document.createComment(value); }
createElement(name: string, namespace?: string|null): RElement {
return document.createElement(name);
}
createText(value: string): RText { return document.createTextNode(value); }
appendChild(parent: RElement, newChild: RNode): void { parent.appendChild(newChild); }
insertBefore(parent: RNode, newChild: RNode, refChild: RNode|null): void {
parent.insertBefore(newChild, refChild, false);
}
removeChild(parent: RElement, oldChild: RNode): void { parent.removeChild(oldChild); }
selectRootElement(selectorOrNode: string|any): RElement {
return ({} as any);
}
parentNode(node: RNode): RElement|null { return node.parentNode as RElement; }
nextSibling(node: RNode): RNode|null { return node.nextSibling; }
setAttribute(el: RElement, name: string, value: string, namespace?: string|null): void {}
removeAttribute(el: RElement, name: string, namespace?: string|null): void {}
addClass(el: RElement, name: string): void {}
removeClass(el: RElement, name: string): void {}
setStyle(
el: RElement, style: string, value: any,
flags?: RendererStyleFlags2|RendererStyleFlags3): void {}
removeStyle(el: RElement, style: string, flags?: RendererStyleFlags2|RendererStyleFlags3): void {}
setProperty(el: RElement, name: string, value: any): void {}
setValue(node: RText, value: string): void {}

// TODO(misko): Deprecate in favor of addEventListener/removeEventListener
listen(target: RNode, eventName: string, callback: (event: any) => boolean | void): () => void {
return () => {};
}
}
}
65 changes: 63 additions & 2 deletions packages/core/test/render3/render_util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {Injector, SWITCH_INJECTOR_FACTORY__POST_R3__ as R3_INJECTOR_FACTORY} fro
import {SWITCH_ELEMENT_REF_FACTORY__POST_R3__ as R3_ELEMENT_REF_FACTORY} from '../../src/linker/element_ref';
import {SWITCH_TEMPLATE_REF_FACTORY__POST_R3__ as R3_TEMPLATE_REF_FACTORY} from '../../src/linker/template_ref';
import {SWITCH_VIEW_CONTAINER_REF_FACTORY__POST_R3__ as R3_VIEW_CONTAINER_REF_FACTORY} from '../../src/linker/view_container_ref';
import {SWITCH_RENDERER2_FACTORY__POST_R3__ as R3_RENDERER2_FACTORY} from '../../src/render/api';
import {RendererStyleFlags2, RendererType2, SWITCH_RENDERER2_FACTORY__POST_R3__ as R3_RENDERER2_FACTORY} from '../../src/render/api';
import {CreateComponentOptions} from '../../src/render3/component';
import {getDirectivesAtNodeIndex, getLContext, isComponentInstance} from '../../src/render3/context_discovery';
import {extractDirectiveDef, extractPipeDef} from '../../src/render3/definition';
Expand All @@ -28,7 +28,7 @@ import {ComponentTemplate, ComponentType, DirectiveDef, DirectiveType, RenderFla
import {renderTemplate} from '../../src/render3/instructions';
import {DirectiveDefList, DirectiveTypesOrFactory, PipeDef, PipeDefList, PipeTypesOrFactory} from '../../src/render3/interfaces/definition';
import {PlayerHandler} from '../../src/render3/interfaces/player';
import {RElement, RText, Renderer3, RendererFactory3, domRendererFactory3} from '../../src/render3/interfaces/renderer';
import {ProceduralRenderer3, RComment, RElement, RNode, RText, Renderer3, RendererFactory3, RendererStyleFlags3, domRendererFactory3} from '../../src/render3/interfaces/renderer';
import {HEADER_OFFSET, LView} from '../../src/render3/interfaces/view';
import {Sanitizer} from '../../src/sanitization/security';
import {Type} from '../../src/type';
Expand Down Expand Up @@ -354,3 +354,64 @@ export function enableIvyInjectableFactories() {
(Renderer2 as any)[NG_ELEMENT_ID] = () => R3_RENDERER2_FACTORY();
(Injector as any)[NG_ELEMENT_ID] = () => R3_INJECTOR_FACTORY();
}

export class MockRendererFactory implements RendererFactory3 {
lastRenderer: any;
private _spyOnMethods: string[];

constructor(spyOnMethods?: string[]) { this._spyOnMethods = spyOnMethods || []; }

createRenderer(hostElement: RElement|null, rendererType: RendererType2|null): Renderer3 {
const renderer = this.lastRenderer = new MockRenderer(this._spyOnMethods);
return renderer;
}
}

class MockRenderer implements ProceduralRenderer3 {
public spies: {[methodName: string]: any} = {};

constructor(spyOnMethods: string[]) {
spyOnMethods.forEach(methodName => {
this.spies[methodName] = spyOn(this as any, methodName).and.callThrough();
});
}

destroy(): void {}
createComment(value: string): RComment { return document.createComment(value); }
createElement(name: string, namespace?: string|null): RElement {
return document.createElement(name);
}
createText(value: string): RText { return document.createTextNode(value); }
appendChild(parent: RElement, newChild: RNode): void { parent.appendChild(newChild); }
insertBefore(parent: RNode, newChild: RNode, refChild: RNode|null): void {
parent.insertBefore(newChild, refChild, false);
}
removeChild(parent: RElement, oldChild: RNode): void { parent.removeChild(oldChild); }
selectRootElement(selectorOrNode: string|any): RElement {
return ({} as any);
}
parentNode(node: RNode): RElement|null { return node.parentNode as RElement; }
nextSibling(node: RNode): RNode|null { return node.nextSibling; }
setAttribute(el: RElement, name: string, value: string, namespace?: string|null): void {
// set all synthetic attributes as properties
if (name[0] === '@') {
this.setProperty(el, name, value);
} else {
el.setAttribute(name, value);
}
}
removeAttribute(el: RElement, name: string, namespace?: string|null): void {}
addClass(el: RElement, name: string): void {}
removeClass(el: RElement, name: string): void {}
setStyle(
el: RElement, style: string, value: any,
flags?: RendererStyleFlags2|RendererStyleFlags3): void {}
removeStyle(el: RElement, style: string, flags?: RendererStyleFlags2|RendererStyleFlags3): void {}
setProperty(el: RElement, name: string, value: any): void { (el as any)[name] = value; }
setValue(node: RText, value: string): void { node.textContent = value; }

// TODO(misko): Deprecate in favor of addEventListener/removeEventListener
listen(target: RNode, eventName: string, callback: (event: any) => boolean | void): () => void {
return () => {};
}
}
132 changes: 65 additions & 67 deletions packages/platform-browser/animations/test/animation_renderer_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,77 +195,75 @@ import {el} from '../../testing/src/browser_util';
});
});

fixmeIvy(
`FW-801: Components with animations throw with "Cannot read property 'hostElement' of undefined" error`)
.it('should only queue up dom removals if the element itself contains a valid leave animation',
() => {
@Component({
selector: 'my-cmp',
template: `
it('should only queue up dom removals if the element itself contains a valid leave animation',
() => {
@Component({
selector: 'my-cmp',
template: `
<div #elm1 *ngIf="exp1"></div>
<div #elm2 @animation1 *ngIf="exp2"></div>
<div #elm3 @animation2 *ngIf="exp3"></div>
`,
animations: [
trigger('animation1', [transition('a => b', [])]),
trigger('animation2', [transition(':leave', [])]),
]
})
class Cmp {
exp1: any = true;
exp2: any = true;
exp3: any = true;

@ViewChild('elm1') public elm1: any;

@ViewChild('elm2') public elm2: any;

@ViewChild('elm3') public elm3: any;
}

TestBed.configureTestingModule({
providers: [{provide: AnimationEngine, useClass: InjectableAnimationEngine}],
declarations: [Cmp]
});

const engine = TestBed.get(AnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;

fixture.detectChanges();
const elm1 = cmp.elm1;
const elm2 = cmp.elm2;
const elm3 = cmp.elm3;
assertHasParent(elm1);
assertHasParent(elm2);
assertHasParent(elm3);
engine.flush();
finishPlayers(engine.players);

cmp.exp1 = false;
fixture.detectChanges();
assertHasParent(elm1, false);
assertHasParent(elm2);
assertHasParent(elm3);
engine.flush();
expect(engine.players.length).toEqual(0);

cmp.exp2 = false;
fixture.detectChanges();
assertHasParent(elm1, false);
assertHasParent(elm2, false);
assertHasParent(elm3);
engine.flush();
expect(engine.players.length).toEqual(0);

cmp.exp3 = false;
fixture.detectChanges();
assertHasParent(elm1, false);
assertHasParent(elm2, false);
assertHasParent(elm3);
engine.flush();
expect(engine.players.length).toEqual(1);
});
animations: [
trigger('animation1', [transition('a => b', [])]),
trigger('animation2', [transition(':leave', [])]),
]
})
class Cmp {
exp1: any = true;
exp2: any = true;
exp3: any = true;

@ViewChild('elm1') public elm1: any;

@ViewChild('elm2') public elm2: any;

@ViewChild('elm3') public elm3: any;
}

TestBed.configureTestingModule({
providers: [{provide: AnimationEngine, useClass: InjectableAnimationEngine}],
declarations: [Cmp]
});

const engine = TestBed.get(AnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;

fixture.detectChanges();
const elm1 = cmp.elm1;
const elm2 = cmp.elm2;
const elm3 = cmp.elm3;
assertHasParent(elm1);
assertHasParent(elm2);
assertHasParent(elm3);
engine.flush();
finishPlayers(engine.players);

cmp.exp1 = false;
fixture.detectChanges();
assertHasParent(elm1, false);
assertHasParent(elm2);
assertHasParent(elm3);
engine.flush();
expect(engine.players.length).toEqual(0);

cmp.exp2 = false;
fixture.detectChanges();
assertHasParent(elm1, false);
assertHasParent(elm2, false);
assertHasParent(elm3);
engine.flush();
expect(engine.players.length).toEqual(0);

cmp.exp3 = false;
fixture.detectChanges();
assertHasParent(elm1, false);
assertHasParent(elm2, false);
assertHasParent(elm3);
engine.flush();
expect(engine.players.length).toEqual(1);
});
});
});

Expand Down

0 comments on commit 37c05bd

Please sign in to comment.