From 37c05bd575d4b73251283d782c6c340cc134c6fe Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Mon, 10 Dec 2018 23:40:19 -0800 Subject: [PATCH] fix(ivy): avoid destroy renderer method invocation for child views (#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 --- .../core/src/render3/node_manipulation.ts | 3 +- packages/core/test/render3/component_spec.ts | 61 +++++++++- .../core/test/render3/integration_spec.ts | 62 +--------- packages/core/test/render3/render_util.ts | 65 +++++++++- .../test/animation_renderer_spec.ts | 114 +++++++++--------- 5 files changed, 185 insertions(+), 120 deletions(-) diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 2bd2c74b2b..d79442d56e 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -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(); } diff --git a/packages/core/test/render3/component_spec.ts b/packages/core/test/render3/component_spec.ts index d0e9a4ead6..1a6f545c86 100644 --- a/packages/core/test/render3/component_spec.ts +++ b/packages/core/test/render3/component_spec.ts @@ -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 { @@ -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], + /** + *
Root view
+ *
Child view
+ */ + 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[]}) { diff --git a/packages/core/test/render3/integration_spec.ts b/packages/core/test/render3/integration_spec.ts index d9a1a97cda..f7de4aa0f8 100644 --- a/packages/core/test/render3/integration_spec.ts +++ b/packages/core/test/render3/integration_spec.ts @@ -7,12 +7,12 @@ */ 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'; @@ -20,7 +20,7 @@ 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'; @@ -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 () => {}; - } -} +} \ No newline at end of file diff --git a/packages/core/test/render3/render_util.ts b/packages/core/test/render3/render_util.ts index 332ecee319..226ff1e078 100644 --- a/packages/core/test/render3/render_util.ts +++ b/packages/core/test/render3/render_util.ts @@ -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'; @@ -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'; @@ -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 () => {}; + } +} \ No newline at end of file diff --git a/packages/platform-browser/animations/test/animation_renderer_spec.ts b/packages/platform-browser/animations/test/animation_renderer_spec.ts index 8dafb4e600..733aa52763 100644 --- a/packages/platform-browser/animations/test/animation_renderer_spec.ts +++ b/packages/platform-browser/animations/test/animation_renderer_spec.ts @@ -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: `
`, - animations: [ - trigger('animation1', [transition('a => b', [])]), - trigger('animation2', [transition(':leave', [])]), - ] - }) - class Cmp { - exp1: any = true; - exp2: any = true; - exp3: any = true; + 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('elm1') public elm1: any; - @ViewChild('elm2') public elm2: any; + @ViewChild('elm2') public elm2: any; - @ViewChild('elm3') public elm3: any; - } + @ViewChild('elm3') public elm3: any; + } - TestBed.configureTestingModule({ - providers: [{provide: AnimationEngine, useClass: InjectableAnimationEngine}], - declarations: [Cmp] - }); + TestBed.configureTestingModule({ + providers: [{provide: AnimationEngine, useClass: InjectableAnimationEngine}], + declarations: [Cmp] + }); - const engine = TestBed.get(AnimationEngine); - const fixture = TestBed.createComponent(Cmp); - const cmp = fixture.componentInstance; + 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); + 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.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.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); - }); + cmp.exp3 = false; + fixture.detectChanges(); + assertHasParent(elm1, false); + assertHasParent(elm2, false); + assertHasParent(elm3); + engine.flush(); + expect(engine.players.length).toEqual(1); + }); }); });