From f1f408ef3def8346420d67fb63e52d5703cb9bdb Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 8 Jan 2021 14:29:45 +0200 Subject: [PATCH 1/2] fix(core): allow EmbeddedViewRef context to be updated Currently `EmbeddedViewRef.context` is read-only which means that the only way to update it is to mutate the object which can lead to some undesirable outcomes if the template and the context are provided by an external consumer (see #24515). These changes make the property writeable since there doesn't appear to be a specific reason why it was readonly to begin with. --- goldens/public-api/core/core.d.ts | 2 +- packages/core/src/linker/view_ref.ts | 2 +- packages/core/src/render3/view_ref.ts | 4 + packages/core/src/view/refs.ts | 4 + .../core/test/acceptance/template_ref_spec.ts | 83 +++++++++++++++++++ 5 files changed, 93 insertions(+), 2 deletions(-) diff --git a/goldens/public-api/core/core.d.ts b/goldens/public-api/core/core.d.ts index 56dfa25b8c8f9..a9f381b6390dd 100644 --- a/goldens/public-api/core/core.d.ts +++ b/goldens/public-api/core/core.d.ts @@ -301,7 +301,7 @@ export declare class ElementRef { } export declare abstract class EmbeddedViewRef extends ViewRef { - abstract get context(): C; + abstract context: C; abstract get rootNodes(): any[]; } diff --git a/packages/core/src/linker/view_ref.ts b/packages/core/src/linker/view_ref.ts index bafc35a4880eb..315f86e6d7712 100644 --- a/packages/core/src/linker/view_ref.ts +++ b/packages/core/src/linker/view_ref.ts @@ -93,7 +93,7 @@ export abstract class EmbeddedViewRef extends ViewRef { /** * The context for this view, inherited from the anchor element. */ - abstract get context(): C; + abstract context: C; /** * The root nodes for this embedded view. diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index b88e8b51ba802..8f7fdc72fa08b 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -61,6 +61,10 @@ export class ViewRef implements viewEngine_EmbeddedViewRef, viewEngine_Int return this._lView[CONTEXT] as T; } + set context(value: T) { + this._lView[CONTEXT] = value; + } + get destroyed(): boolean { return (this._lView[FLAGS] & LViewFlags.Destroyed) === LViewFlags.Destroyed; } diff --git a/packages/core/src/view/refs.ts b/packages/core/src/view/refs.ts index 7d219ace1c253..b724fcc8003e6 100644 --- a/packages/core/src/view/refs.ts +++ b/packages/core/src/view/refs.ts @@ -264,6 +264,10 @@ export class ViewRef_ implements EmbeddedViewRef, InternalViewRef { return this._view.context; } + set context(value: any) { + this._view.context = value; + } + get destroyed(): boolean { return (this._view.state & ViewState.Destroyed) !== 0; } diff --git a/packages/core/test/acceptance/template_ref_spec.ts b/packages/core/test/acceptance/template_ref_spec.ts index ab14c0c765d39..84ed32d582858 100644 --- a/packages/core/test/acceptance/template_ref_spec.ts +++ b/packages/core/test/acceptance/template_ref_spec.ts @@ -273,4 +273,87 @@ describe('TemplateRef', () => { }); }); }); + + describe('context', () => { + @Component({ + template: ` + {{name}} + + ` + }) + class App { + @ViewChild('templateRef') templateRef!: TemplateRef; + @ViewChild('containerRef', {read: ViewContainerRef}) containerRef!: ViewContainerRef; + } + + it('should update if the context of a view ref is mutated', () => { + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + const context = {name: 'Frodo'}; + const viewRef = fixture.componentInstance.templateRef.createEmbeddedView(context); + fixture.componentInstance.containerRef.insert(viewRef); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('Frodo'); + + context.name = 'Bilbo'; + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('Bilbo'); + }); + + it('should update if the context of a view ref is replaced', () => { + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + const viewRef = fixture.componentInstance.templateRef.createEmbeddedView({name: 'Frodo'}); + fixture.componentInstance.containerRef.insert(viewRef); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('Frodo'); + + viewRef.context = {name: 'Bilbo'}; + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('Bilbo'); + }); + + it('should use the latest context information inside template listeners', () => { + const events: string[] = []; + + @Component({ + template: ` + + + + + ` + }) + class ListenerTest { + @ViewChild('templateRef') templateRef!: TemplateRef; + @ViewChild('containerRef', {read: ViewContainerRef}) containerRef!: ViewContainerRef; + + log(name: string) { + events.push(name); + } + } + + TestBed.configureTestingModule({declarations: [ListenerTest]}); + const fixture = TestBed.createComponent(ListenerTest); + fixture.detectChanges(); + const viewRef = fixture.componentInstance.templateRef.createEmbeddedView({name: 'Frodo'}); + fixture.componentInstance.containerRef.insert(viewRef); + fixture.detectChanges(); + + const button = fixture.nativeElement.querySelector('button'); + button.click(); + expect(events).toEqual(['Frodo']); + + viewRef.context = {name: 'Bilbo'}; + fixture.detectChanges(); + button.click(); + expect(events).toEqual(['Frodo', 'Bilbo']); + }); + }); }); From f175a2e11c91082a11c3983eb69ddb61ddcf2ac9 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 23 Jan 2021 11:13:05 +0100 Subject: [PATCH 2/2] fix(common): avoid mutating context object in NgTemplateOutlet 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. --- 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 57658768860df..fea778e1394d5 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 d2b53b1bd8a76..4ddb84391b119 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 865f8c40daf24..ee3132180ebba 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]})