From d3705b3284113f752ee05e9f0d2f6e75c723ea5b Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 23 Jan 2021 11:13:05 +0100 Subject: [PATCH] fix(common): avoid mutating context object in NgTemplateOutlet (#40360) 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 --- goldens/size-tracking/aio-payloads.json | 2 +- .../src/directives/ng_template_outlet.ts | 45 ++----------------- .../directives/ng_template_outlet_spec.ts | 40 +++++++++++++++-- 3 files changed, 42 insertions(+), 45 deletions(-) diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index 5765876886..fea778e139 100755 --- a/goldens/size-tracking/aio-payloads.json +++ b/goldens/size-tracking/aio-payloads.json @@ -26,4 +26,4 @@ } } } -} \ No newline at end of file +} diff --git a/packages/common/src/directives/ng_template_outlet.ts b/packages/common/src/directives/ng_template_outlet.ts index d2b53b1bd8..4ddb84391b 100644 --- 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)) { - (this._viewRef!.context)[propName] = (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 index 865f8c40da..ee3132180e 100644 --- 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 = `:{{foo}}` + ``; @@ -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: ` + {{name}} + + | + + ` +}) +class MultiContextComponent { + context1: {name: string}|undefined; + context2: {name: string}|undefined; +} + function createTestComponent(template: string): ComponentFixture { return TestBed.overrideComponent(TestComponent, {set: {template: template}}) .configureTestingModule({schemas: [NO_ERRORS_SCHEMA]})