fix(common): avoid mutating context object in NgTemplateOutlet (#40360)

crisbeto · atscott · commit d3705b328411 · 2021-02-23T08:14:02.000-08:00
Currently `NgTemplateOutlet` recreates its view if its template is swapped out or a context object with a different shape is passed in. If an object with the same shape is passed in, we preserve the old view and we mutate the previous object. This mutation of the original object can be undesirable if two objects with the same shape are swapped between two different template outlets. The current behavior is a result of a limitation in `core` where the `context` of an embedded view is read-only, however a previous commit made it writeable. These changes resolve the context mutation issue and clean up a bunch of unnecessary logic from `NgTemplateOutlet` by taking advantage of the earlier change. Fixes #24515. PR Close #40360
diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json
@@ -26,4 +26,4 @@
       }
     }
   }
-}
+}
diff --git a/packages/common/src/directives/ng_template_outlet.ts b/packages/common/src/directives/ng_template_outlet.ts
@@ -52,9 +52,7 @@ export class NgTemplateOutlet implements OnChanges {
   constructor(private _viewContainerRef: ViewContainerRef) {}
 
   ngOnChanges(changes: SimpleChanges) {
-    const recreateView = this._shouldRecreateView(changes);
-
-    if (recreateView) {
+    if (changes['ngTemplateOutlet']) {
       const viewContainerRef = this._viewContainerRef;
 
       if (this._viewRef) {
@@ -64,44 +62,9 @@ export class NgTemplateOutlet implements OnChanges {
       this._viewRef = this.ngTemplateOutlet ?
           viewContainerRef.createEmbeddedView(this.ngTemplateOutlet, this.ngTemplateOutletContext) :
           null;
-    } 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
-   *
-   * We 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;
-    }
-    return true;
-  }
-
-  private _updateExistingContext(ctx: Object): void {
-    for (let propName of Object.keys(ctx)) {
-      (<any>this._viewRef!.context)[propName] = (<any>this.ngTemplateOutletContext)[propName];
+    } else if (
+        this._viewRef && changes['ngTemplateOutletContext'] && this.ngTemplateOutletContext) {
+      this._viewRef.context = this.ngTemplateOutletContext;
     }
   }
 }
diff --git a/packages/common/test/directives/ng_template_outlet_spec.ts b/packages/common/test/directives/ng_template_outlet_spec.ts
@@ -29,7 +29,7 @@ describe('NgTemplateOutlet', () => {
 
   beforeEach(() => {
     TestBed.configureTestingModule({
-      declarations: [TestComponent, CaptureTplRefs, DestroyableCmpt],
+      declarations: [TestComponent, CaptureTplRefs, DestroyableCmpt, MultiContextComponent],
       imports: [CommonModule],
       providers: [DestroyedSpyService]
     });
@@ -142,7 +142,7 @@ describe('NgTemplateOutlet', () => {
     expect(spyService.destroyed).toBeFalsy();
   });
 
-  it('should recreate embedded view when context shape changes', () => {
+  it('should update but not destroy 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>`;
@@ -155,7 +155,7 @@ describe('NgTemplateOutlet', () => {
 
     fixture.componentInstance.context = {foo: 'baz', other: true};
     detectChangesAndExpectText('Content to destroy:baz');
-    expect(spyService.destroyed).toBeTruthy();
+    expect(spyService.destroyed).toBeFalsy();
   });
 
   it('should destroy embedded view when context value changes and templateRef becomes undefined', () => {
@@ -241,6 +241,27 @@ describe('NgTemplateOutlet', () => {
          detectChangesAndExpectText('foo');
        }).not.toThrow();
      }));
+
+  it('should not mutate context object if two contexts with an identical shape are swapped', () => {
+    fixture = TestBed.createComponent(MultiContextComponent);
+    const {componentInstance, nativeElement} = fixture;
+    componentInstance.context1 = {name: 'one'};
+    componentInstance.context2 = {name: 'two'};
+    fixture.detectChanges();
+
+    expect(nativeElement.textContent.trim()).toBe('one | two');
+    expect(componentInstance.context1).toEqual({name: 'one'});
+    expect(componentInstance.context2).toEqual({name: 'two'});
+
+    const temp = componentInstance.context1;
+    componentInstance.context1 = componentInstance.context2;
+    componentInstance.context2 = temp;
+    fixture.detectChanges();
+
+    expect(nativeElement.textContent.trim()).toBe('two | one');
+    expect(componentInstance.context1).toEqual({name: 'two'});
+    expect(componentInstance.context2).toEqual({name: 'one'});
+  });
 });
 
 @Injectable()
@@ -271,6 +292,19 @@ class TestComponent {
   value = 'bar';
 }
 
+@Component({
+  template: `
+  <ng-template #template let-name="name">{{name}}</ng-template>
+  <ng-template [ngTemplateOutlet]="template" [ngTemplateOutletContext]="context1"></ng-template>
+  |
+  <ng-template [ngTemplateOutlet]="template" [ngTemplateOutletContext]="context2"></ng-template>
+  `
+})
+class MultiContextComponent {
+  context1: {name: string}|undefined;
+  context2: {name: string}|undefined;
+}
+
 function createTestComponent(template: string): ComponentFixture<TestComponent> {
   return TestBed.overrideComponent(TestComponent, {set: {template: template}})
       .configureTestingModule({schemas: [NO_ERRORS_SCHEMA]})

Original file line number	Diff line number	Diff line change
`@@ -26,4 +26,4 @@`
`26`	`26`	`}`
`27`	`27`	`}`
`28`	`28`	`}`
`29`		`-}`
	`29`	`+}`