fix(common): don't recreate view when context shape doesn't change (angular#18277)

pkozlowski-opensource · vicb · commit 685cc26ab2dc · 2017-08-07T14:31:26.000-07:00
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 angular#13407
diff --git 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 (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;
+    }
+  }
 
-    if (this.ngTemplateOutlet) {
-      this._viewRef = this._viewContainerRef.createEmbeddedView(
-          this.ngTemplateOutlet, this.ngTemplateOutletContext);
+  private _updateExistingContext(ctx: Object): void {
+    for (let propName of Object.keys(ctx)) {
+      (<any>this._viewRef.context)[propName] = (<any>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
@@ -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 =
+          `<ng-template let-foo="foo" #tpl><destroyable-cmpt></destroyable-cmpt>:{{foo}}</ng-template>` +
+          `<ng-template [ngTemplateOutlet]="tpl" [ngTemplateOutletContext]="{foo: value}"></ng-template>`;
+
+      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 =
+          `<ng-template let-foo="foo" #tpl><destroyable-cmpt></destroyable-cmpt>:{{foo}}</ng-template>` +
+          `<ng-template [ngTemplateOutlet]="tpl" [ngTemplateOutletContext]="context"></ng-template>`;
+
+      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 =
+             `<ng-template let-foo="foo" #tpl><destroyable-cmpt></destroyable-cmpt>:{{foo}}</ng-template>` +
+             `<ng-template [ngTemplateOutlet]="value === 'bar' ? tpl : undefined" [ngTemplateOutletContext]="{foo: value}"></ng-template>`;
+
+         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 = `<ng-template let-foo="foo" #tpl>{{foo}}</ng-template>` +
+             `<ng-template [ngTemplateOutlet]="tpl" [ngTemplateOutletContext]="value === 'bar' ? null : undefined"></ng-template>`;
+
+         fixture = createTestComponent(template);
+         detectChangesAndExpectText('');
+
+         fixture.componentInstance.value = 'baz';
+         detectChangesAndExpectText('');
+       });
+
+    it('should not try to update null / undefined context when template changes', () => {
+      const template = `<ng-template let-foo="foo" #tpl1>{{foo}}</ng-template>` +
+          `<ng-template let-foo="foo" #tpl2>{{foo}}</ng-template>` +
+          `<ng-template [ngTemplateOutlet]="value === 'bar' ? tpl1 : tpl2" [ngTemplateOutletContext]="value === 'bar' ? null : undefined"></ng-template>`;
+
+      fixture = createTestComponent(template);
+      detectChangesAndExpectText('');
+
+      fixture.componentInstance.value = 'baz';
+      detectChangesAndExpectText('');
+    });
+
+    it('should not try to update context on undefined view', () => {
+      const template = `<ng-template let-foo="foo" #tpl>{{foo}}</ng-template>` +
+          `<ng-template [ngTemplateOutlet]="value === 'bar' ? null : undefined" [ngTemplateOutletContext]="{foo: value}"></ng-template>`;
+
+      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<TemplateRef<any>>;
@@ -137,6 +231,7 @@ class CaptureTplRefs {
 class TestComponent {
   currentTplRef: TemplateRef<any>;
   context: any = {foo: 'bar'};
+  value = 'bar';
 }
 
 function createTestComponent(template: string): ComponentFixture<TestComponent> {
