From 685cc26ab2dc7f8d836ec932a43d45edfa0f8e97 Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Mon, 7 Aug 2017 23:31:26 +0200 Subject: [PATCH] fix(common): don't recreate view when context shape doesn't change (#18277) Problem description: when using ngTemplateOutlet with context as an object literal in a template and binding to the context's property the embedded view would get re-created even if context object remains essentially the same (the same shape, just update to one properties). This happens since currently change detection will re-create object references when an object literal is used and one of its properties gets updated through a binding. Solution: this commit changes ngTemplateOutlet logic so we take context object shape into account before deciding if we should re-create view or just update existing context. Fixes #13407 --- .../src/directives/ng_template_outlet.ts | 59 ++++++++-- .../directives/ng_template_outlet_spec.ts | 105 +++++++++++++++++- 2 files changed, 152 insertions(+), 12 deletions(-) diff --git a/packages/common/src/directives/ng_template_outlet.ts b/packages/common/src/directives/ng_template_outlet.ts index ccfa8ecee4..800eed23ec 100644 --- a/packages/common/src/directives/ng_template_outlet.ts +++ b/packages/common/src/directives/ng_template_outlet.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Directive, EmbeddedViewRef, Input, OnChanges, SimpleChanges, TemplateRef, ViewContainerRef} from '@angular/core'; +import {Directive, EmbeddedViewRef, Input, OnChanges, SimpleChange, SimpleChanges, TemplateRef, ViewContainerRef} from '@angular/core'; /** * @ngModule CommonModule @@ -49,13 +49,58 @@ export class NgTemplateOutlet implements OnChanges { set ngOutletContext(context: Object) { this.ngTemplateOutletContext = context; } ngOnChanges(changes: SimpleChanges) { - if (this._viewRef) { - this._viewContainerRef.remove(this._viewContainerRef.indexOf(this._viewRef)); - } + const recreateView = this._shouldRecreateView(changes); - if (this.ngTemplateOutlet) { - this._viewRef = this._viewContainerRef.createEmbeddedView( - this.ngTemplateOutlet, this.ngTemplateOutletContext); + if (recreateView) { + if (this._viewRef) { + this._viewContainerRef.remove(this._viewContainerRef.indexOf(this._viewRef)); + } + + if (this.ngTemplateOutlet) { + this._viewRef = this._viewContainerRef.createEmbeddedView( + this.ngTemplateOutlet, this.ngTemplateOutletContext); + } + } else { + if (this._viewRef && this.ngTemplateOutletContext) { + this._updateExistingContext(this.ngTemplateOutletContext); + } + } + } + + /** + * We need to re-create existing embedded view if: + * - templateRef has changed + * - context has changes + * + * To mark context object as changed when the corresponding object + * shape changes (new properties are added or existing properties are removed). + * In other words we consider context with the same properties as "the same" even + * if object reference changes (see https://github.com/angular/angular/issues/13407). + */ + private _shouldRecreateView(changes: SimpleChanges): boolean { + const ctxChange = changes['ngTemplateOutletContext']; + return !!changes['ngTemplateOutlet'] || (ctxChange && this._hasContextShapeChanged(ctxChange)); + } + + private _hasContextShapeChanged(ctxChange: SimpleChange): boolean { + const prevCtxKeys = Object.keys(ctxChange.previousValue || {}); + const currCtxKeys = Object.keys(ctxChange.currentValue || {}); + + if (prevCtxKeys.length === currCtxKeys.length) { + for (let propName of currCtxKeys) { + if (prevCtxKeys.indexOf(propName) === -1) { + return true; + } + } + return false; + } else { + return true; + } + } + + private _updateExistingContext(ctx: Object): void { + for (let propName of Object.keys(ctx)) { + (this._viewRef.context)[propName] = (this.ngTemplateOutletContext)[propName]; } } } diff --git a/packages/common/test/directives/ng_template_outlet_spec.ts b/packages/common/test/directives/ng_template_outlet_spec.ts index 8f1d25ac95..13135629cd 100644 --- a/packages/common/test/directives/ng_template_outlet_spec.ts +++ b/packages/common/test/directives/ng_template_outlet_spec.ts @@ -7,7 +7,7 @@ */ import {CommonModule} from '@angular/common'; -import {Component, ContentChildren, Directive, NO_ERRORS_SCHEMA, QueryList, TemplateRef} from '@angular/core'; +import {Component, ContentChildren, Directive, Injectable, NO_ERRORS_SCHEMA, OnDestroy, QueryList, TemplateRef} from '@angular/core'; import {ComponentFixture, TestBed, async} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -26,11 +26,9 @@ export function main() { beforeEach(() => { TestBed.configureTestingModule({ - declarations: [ - TestComponent, - CaptureTplRefs, - ], + declarations: [TestComponent, CaptureTplRefs, DestroyableCmpt], imports: [CommonModule], + providers: [DestroyedSpyService] }); }); @@ -125,9 +123,105 @@ export function main() { fixture.componentInstance.context = {shawshank: 'was here'}; detectChangesAndExpectText('was here'); })); + + it('should update but not destroy embedded view when context values change', () => { + const template = + `:{{foo}}` + + ``; + + fixture = createTestComponent(template); + const spyService = fixture.debugElement.injector.get(DestroyedSpyService); + + detectChangesAndExpectText('Content to destroy:bar'); + expect(spyService.destroyed).toBeFalsy(); + + fixture.componentInstance.value = 'baz'; + detectChangesAndExpectText('Content to destroy:baz'); + expect(spyService.destroyed).toBeFalsy(); + }); + + it('should recreate embedded view when context shape changes', () => { + const template = + `:{{foo}}` + + ``; + + fixture = createTestComponent(template); + const spyService = fixture.debugElement.injector.get(DestroyedSpyService); + + detectChangesAndExpectText('Content to destroy:bar'); + expect(spyService.destroyed).toBeFalsy(); + + fixture.componentInstance.context = {foo: 'baz', other: true}; + detectChangesAndExpectText('Content to destroy:baz'); + expect(spyService.destroyed).toBeTruthy(); + }); + + it('should destroy embedded view when context value changes and templateRef becomes undefined', + () => { + const template = + `:{{foo}}` + + ``; + + fixture = createTestComponent(template); + const spyService = fixture.debugElement.injector.get(DestroyedSpyService); + + detectChangesAndExpectText('Content to destroy:bar'); + expect(spyService.destroyed).toBeFalsy(); + + fixture.componentInstance.value = 'baz'; + detectChangesAndExpectText(''); + expect(spyService.destroyed).toBeTruthy(); + }); + + it('should not try to update null / undefined context when context changes but template stays the same', + () => { + const template = `{{foo}}` + + ``; + + fixture = createTestComponent(template); + detectChangesAndExpectText(''); + + fixture.componentInstance.value = 'baz'; + detectChangesAndExpectText(''); + }); + + it('should not try to update null / undefined context when template changes', () => { + const template = `{{foo}}` + + `{{foo}}` + + ``; + + fixture = createTestComponent(template); + detectChangesAndExpectText(''); + + fixture.componentInstance.value = 'baz'; + detectChangesAndExpectText(''); + }); + + it('should not try to update context on undefined view', () => { + const template = `{{foo}}` + + ``; + + fixture = createTestComponent(template); + detectChangesAndExpectText(''); + + fixture.componentInstance.value = 'baz'; + detectChangesAndExpectText(''); + }); }); } +@Injectable() +class DestroyedSpyService { + destroyed = false; +} + +@Component({selector: 'destroyable-cmpt', template: 'Content to destroy'}) +class DestroyableCmpt implements OnDestroy { + constructor(private _spyService: DestroyedSpyService) {} + + ngOnDestroy(): void { this._spyService.destroyed = true; } +} + @Directive({selector: 'tpl-refs', exportAs: 'tplRefs'}) class CaptureTplRefs { @ContentChildren(TemplateRef) tplRefs: QueryList>; @@ -137,6 +231,7 @@ class CaptureTplRefs { class TestComponent { currentTplRef: TemplateRef; context: any = {foo: 'bar'}; + value = 'bar'; } function createTestComponent(template: string): ComponentFixture {