From 404c8d0d88b88ae3ab5a80609d508fb2ecdf0d27 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 3 Jul 2021 11:59:58 +0200 Subject: [PATCH] fix(compiler): incorrect context object being referenced from listener instructions inside embedded views (#42755) Currently unless a listener inside of an embedded view tries to reference something from the parent view, or if the reference is a local ref, we don't generate the view restoration instructions and we allow for the value to be picked up from the context object in the function parameters. The problem is that the listener is only run during creation mode and the context object may have been swapped out afterwards. These changes fix the issue by always generating the view restoration instructions for listeners inside templates. Fixes #42698. PR Close #42755 --- .../GOLDEN_PARTIAL.js | 48 +++++++++++++++++++ .../r3_view_compiler_listener/TEST_CASES.json | 17 +++++++ .../embedded_view_listener_context.ts | 16 +++++++ ...embedded_view_listener_context_template.js | 13 +++++ .../src/compiler_util/expression_converter.ts | 4 +- .../compiler/src/render3/view/template.ts | 27 +++++------ .../core/test/acceptance/listener_spec.ts | 41 ++++++++++++++++ 7 files changed, 148 insertions(+), 18 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context.ts create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context_template.js diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js index b2ce2b31a0..5e698d0a5f 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js @@ -636,3 +636,51 @@ export declare class MyModule { static ɵinj: i0.ɵɵInjectorDeclaration; } +/**************************************************************************************************** + * PARTIAL FILE: embedded_view_listener_context.js + ****************************************************************************************************/ +import { Component, NgModule } from '@angular/core'; +import * as i0 from "@angular/core"; +export class MyComponent { +} +MyComponent.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyComponent, deps: [], target: i0.ɵɵFactoryTarget.Component }); +MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", ngImport: i0, template: ` + + + + `, isInline: true }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyComponent, decorators: [{ + type: Component, + args: [{ + selector: 'my-component', + template: ` + + + + ` + }] + }] }); +export class MyModule { +} +MyModule.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyModule, deps: [], target: i0.ɵɵFactoryTarget.NgModule }); +MyModule.ɵmod = i0.ɵɵngDeclareNgModule({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyModule, declarations: [MyComponent] }); +MyModule.ɵinj = i0.ɵɵngDeclareInjector({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyModule }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyModule, decorators: [{ + type: NgModule, + args: [{ declarations: [MyComponent] }] + }] }); + +/**************************************************************************************************** + * PARTIAL FILE: embedded_view_listener_context.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class MyComponent { + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵcmp: i0.ɵɵComponentDeclaration; +} +export declare class MyModule { + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵinj: i0.ɵɵInjectorDeclaration; +} + diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json index 992de375c8..52cce67833 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json @@ -281,6 +281,23 @@ "failureMessage": "Incorrect template" } ] + }, + { + "description": "should reference correct context in listener inside embedded view", + "inputFiles": [ + "embedded_view_listener_context.ts" + ], + "expectations": [ + { + "files": [ + { + "expected": "embedded_view_listener_context_template.js", + "generated": "embedded_view_listener_context.js" + } + ], + "failureMessage": "Incorrect template" + } + ] } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context.ts new file mode 100644 index 0000000000..72876362f8 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context.ts @@ -0,0 +1,16 @@ +import {Component, NgModule} from '@angular/core'; + +@Component({ + selector: 'my-component', + template: ` + + + + ` +}) +export class MyComponent { +} + +@NgModule({declarations: [MyComponent]}) +export class MyModule { +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context_template.js new file mode 100644 index 0000000000..f903938b9c --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context_template.js @@ -0,0 +1,13 @@ +function MyComponent_ng_template_0_Template(rf, $ctx$) { + if (rf & 1) { + const _r3 = $i0$.ɵɵgetCurrentView(); + $i0$.ɵɵelementStart(0, "button", 0); + $i0$.ɵɵlistener("click", function MyComponent_ng_template_0_Template_button_click_0_listener() { + const restoredCtx = $i0$.ɵɵrestoreView(_r3); + const $obj_r1$ = restoredCtx.$implicit; + return $obj_r1$.value = 1; + }); + $i0$.ɵɵtext(1, "Change"); + $i0$.ɵɵelementEnd(); + } +} diff --git a/packages/compiler/src/compiler_util/expression_converter.ts b/packages/compiler/src/compiler_util/expression_converter.ts index cd0b50f4b4..6911c6d981 100644 --- a/packages/compiler/src/compiler_util/expression_converter.ts +++ b/packages/compiler/src/compiler_util/expression_converter.ts @@ -19,7 +19,7 @@ export interface LocalResolver { getLocal(name: string): o.Expression|null; notifyImplicitReceiverUse(): void; globals?: Set; - maybeRestoreView(retrievalLevel: number, localRefLookup: boolean): void; + maybeRestoreView(): void; } export class ConvertActionBindingResult { @@ -490,7 +490,7 @@ class _AstToIrVisitor implements cdAst.AstVisitor { const value: o.Expression = this._visit(ast.value, _Mode.Expression); if (obj === this._implicitReceiver) { - this._localResolver.maybeRestoreView(0, false); + this._localResolver.maybeRestoreView(); } return convertToStatementIfNeeded(mode, obj.key(key).set(value)); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index e4ed819a7f..9f5d1b6191 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -340,8 +340,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver } // LocalResolver - maybeRestoreView(retrievalLevel: number, localRefLookup: boolean): void { - this._bindingScope.maybeRestoreView(retrievalLevel, localRefLookup); + maybeRestoreView(): void { + this._bindingScope.maybeRestoreView(); } private i18nTranslate( @@ -1656,7 +1656,6 @@ const SHARED_CONTEXT_KEY = '$$shared_ctx$$'; type BindingData = { retrievalLevel: number; lhs: o.Expression; declareLocalCallback?: DeclareLocalVarCallback; declare: boolean; priority: number; - localRef: boolean; }; /** @@ -1701,15 +1700,14 @@ export class BindingScope implements LocalResolver { lhs: value.lhs, declareLocalCallback: value.declareLocalCallback, declare: false, - priority: value.priority, - localRef: value.localRef + priority: value.priority }; // Cache the value locally. this.map.set(name, value); // Possibly generate a shared context var this.maybeGenerateSharedContextVar(value); - this.maybeRestoreView(value.retrievalLevel, value.localRef); + this.maybeRestoreView(); } if (value.declareLocalCallback && !value.declare) { @@ -1754,7 +1752,6 @@ export class BindingScope implements LocalResolver { declare: false, declareLocalCallback: declareLocalCallback, priority: priority, - localRef: localRef || false }); return this; } @@ -1823,24 +1820,22 @@ export class BindingScope implements LocalResolver { }, declare: false, priority: DeclarationPriority.SHARED_CONTEXT, - localRef: false }); } getComponentProperty(name: string): o.Expression { const componentValue = this.map.get(SHARED_CONTEXT_KEY + 0)!; componentValue.declare = true; - this.maybeRestoreView(0, false); + this.maybeRestoreView(); return componentValue.lhs.prop(name); } - maybeRestoreView(retrievalLevel: number, localRefLookup: boolean) { - // We want to restore the current view in listener fns if: - // 1 - we are accessing a value in a parent view, which requires walking the view tree rather - // than using the ctx arg. In this case, the retrieval and binding level will be different. - // 2 - we are looking up a local ref, which requires restoring the view where the local - // ref is stored - if (this.isListenerScope() && (retrievalLevel < this.bindingLevel || localRefLookup)) { + maybeRestoreView() { + // View restoration is required for listener instructions inside embedded views, because + // they only run in creation mode and they can have references to the context object. + // If the context object changes in update mode, the reference will be incorrect, because + // it was established during creation. + if (this.isListenerScope()) { if (!this.parent!.restoreViewVariable) { // parent saves variable to generate a shared `const $s$ = getCurrentView();` instruction this.parent!.restoreViewVariable = o.variable(this.parent!.freshReferenceName()); diff --git a/packages/core/test/acceptance/listener_spec.ts b/packages/core/test/acceptance/listener_spec.ts index 118cbb84ce..597d830d03 100644 --- a/packages/core/test/acceptance/listener_spec.ts +++ b/packages/core/test/acceptance/listener_spec.ts @@ -552,4 +552,45 @@ describe('event listeners', () => { expect(fixture.componentInstance.message).toBe('hello'); }); + + it('should reference the correct context object if it is swapped out', () => { + @Component({ + template: ` + + + + + + ` + }) + class MyComp { + one = {value: 'one'}; + two = {value: 'two'}; + current = this.one; + } + + TestBed.configureTestingModule({declarations: [MyComp], imports: [CommonModule]}); + const fixture = TestBed.createComponent(MyComp); + const instance = fixture.componentInstance; + fixture.detectChanges(); + const button = fixture.nativeElement.querySelector('button'); + + expect(instance.one.value).toBe('one'); + expect(instance.two.value).toBe('two'); + + button.click(); + fixture.detectChanges(); + + expect(instance.one.value).toBe('one!'); + expect(instance.two.value).toBe('two'); + + instance.current = instance.two; + fixture.detectChanges(); + + button.click(); + fixture.detectChanges(); + + expect(instance.one.value).toBe('one!'); + expect(instance.two.value).toBe('two!'); + }); });