From 1810cdf2c3553322407e3c44a5680065958f6f73 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Fri, 9 Nov 2018 15:07:50 -0800 Subject: [PATCH] fix(ivy): compiler should generate restoreView() for local refs in listeners (#27034) PR Close #27034 --- .../r3_view_compiler_listener_spec.ts | 2 + .../compiler/src/render3/view/template.ts | 50 ++++++++++------- packages/core/test/render3/listeners_spec.ts | 55 ++++++++++++++++++- .../forms/test/template_integration_spec.ts | 53 +++++++++--------- 4 files changed, 112 insertions(+), 48 deletions(-) diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts index c07f8f8271..6882162713 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts @@ -206,8 +206,10 @@ describe('compiler compliance: listen()', () => { vars: 0, template: function MyComponent_Template(rf, ctx) { if (rf & 1) { + const $s$ = $r3$.ɵgetCurrentView(); $r3$.ɵelementStart(0, "button", $e0_attrs$); $r3$.ɵlistener("click", function MyComponent_Template_button_click_listener($event) { + $r3$.ɵrestoreView($s$); const $user$ = $r3$.ɵreference(3); return ctx.onClick($user$.value); }); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 6e7ec2d7a3..82fe78e4c3 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -209,7 +209,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver const updateStatements = this._updateCodeFns.map((fn: () => o.Statement) => fn()); // Variable declaration must occur after binding resolution so we can generate context - // instructions that build on each other. e.g. const b = x().$implicit(); const b = x(); + // instructions that build on each other. + // e.g. const b = nextContext().$implicit(); const b = nextContext(); const creationVariables = this._bindingScope.viewSnapshotStatements(); const updateVariables = this._bindingScope.variableDeclarations().concat(this._tempVariables); @@ -1017,16 +1018,16 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver const retrievalLevel = this.level; const lhs = o.variable(variableName); this._bindingScope.set( - retrievalLevel, reference.name, lhs, DeclarationPriority.DEFAULT, - (scope: BindingScope, relativeLevel: number) => { - // e.g. x(2); + retrievalLevel, reference.name, lhs, + DeclarationPriority.DEFAULT, (scope: BindingScope, relativeLevel: number) => { + // e.g. nextContext(2); const nextContextStmt = relativeLevel > 0 ? [generateNextContextExpr(relativeLevel).toStmt()] : []; - // e.g. const $foo$ = r(1); + // e.g. const $foo$ = reference(1); const refExpr = lhs.set(o.importExpr(R3.reference).callFn([o.literal(slot)])); return nextContextStmt.concat(refExpr.toConstDecl()); - }); + }, true); return [reference.name, reference.value]; })); @@ -1201,13 +1202,14 @@ export type DeclareLocalVarCallback = (scope: BindingScope, relativeLevel: numbe const SHARED_CONTEXT_KEY = '$$shared_ctx$$'; /** - * This is used when one refers to variable such as: 'let abc = x(2).$implicit`. + * This is used when one refers to variable such as: 'let abc = nextContext(2).$implicit`. * - key to the map is the string literal `"abc"`. * - value `retrievalLevel` is the level from which this value can be retrieved, which is 2 levels * up in example. * - value `lhs` is the left hand side which is an AST representing `abc`. * - value `declareLocalCallback` is a callback that is invoked when declaring the local. * - value `declare` is true if this value needs to be declared. + * - value `localRef` is true if we are storing a local reference * - value `priority` dictates the sorting priority of this var declaration compared * to other var declarations on the same retrieval level. For example, if there is a * context variable and a local ref accessing the same parent view, the context var @@ -1217,6 +1219,7 @@ type BindingData = { retrievalLevel: number; lhs: o.ReadVarExpr; declareLocalCallback?: DeclareLocalVarCallback; declare: boolean; priority: number; + localRef: boolean; }; /** @@ -1253,14 +1256,15 @@ export class BindingScope implements LocalResolver { lhs: value.lhs, declareLocalCallback: value.declareLocalCallback, declare: false, - priority: value.priority + priority: value.priority, + localRef: value.localRef }; // Cache the value locally. this.map.set(name, value); // Possibly generate a shared context var this.maybeGenerateSharedContextVar(value); - this.maybeRestoreView(value.retrievalLevel); + this.maybeRestoreView(value.retrievalLevel, value.localRef); } if (value.declareLocalCallback && !value.declare) { @@ -1286,10 +1290,11 @@ export class BindingScope implements LocalResolver { * @param lhs AST representing the left hand side of the `let lhs = rhs;`. * @param priority The sorting priority of this var * @param declareLocalCallback The callback to invoke when declaring this local var + * @param localRef Whether or not this is a local ref */ set(retrievalLevel: number, name: string, lhs: o.ReadVarExpr, priority: number = DeclarationPriority.DEFAULT, - declareLocalCallback?: DeclareLocalVarCallback): BindingScope { + declareLocalCallback?: DeclareLocalVarCallback, localRef?: true): BindingScope { !this.map.has(name) || error(`The name ${name} is already defined in scope to be ${this.map.get(name)}`); this.map.set(name, { @@ -1297,7 +1302,8 @@ export class BindingScope implements LocalResolver { lhs: lhs, declare: false, declareLocalCallback: declareLocalCallback, - priority: priority + priority: priority, + localRef: localRef || false }); return this; } @@ -1332,25 +1338,31 @@ export class BindingScope implements LocalResolver { retrievalLevel: retrievalLevel, lhs: lhs, declareLocalCallback: (scope: BindingScope, relativeLevel: number) => { - // const ctx_r0 = x(2); + // const ctx_r0 = nextContext(2); return [lhs.set(generateNextContextExpr(relativeLevel)).toConstDecl()]; }, declare: false, - priority: DeclarationPriority.SHARED_CONTEXT + 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); + this.maybeRestoreView(0, false); return componentValue.lhs.prop(name); } - maybeRestoreView(retrievalLevel: number) { - if (this.isListenerScope() && retrievalLevel < this.bindingLevel) { + 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)) { if (!this.parent !.restoreViewVariable) { - // parent saves variable to generate a shared `const $s$ = gV();` instruction + // parent saves variable to generate a shared `const $s$ = getCurrentView();` instruction this.parent !.restoreViewVariable = o.variable(this.parent !.freshReferenceName()); } this.restoreViewVariable = this.parent !.restoreViewVariable; @@ -1358,14 +1370,14 @@ export class BindingScope implements LocalResolver { } restoreViewStatement(): o.Statement[] { - // rV($state$); + // restoreView($state$); return this.restoreViewVariable ? [instruction(null, R3.restoreView, [this.restoreViewVariable]).toStmt()] : []; } viewSnapshotStatements(): o.Statement[] { - // const $state$ = gV(); + // const $state$ = getCurrentView(); const getCurrentViewInstruction = instruction(null, R3.getCurrentView, []); return this.restoreViewVariable ? [this.restoreViewVariable.set(getCurrentViewInstruction).toConstDecl()] : diff --git a/packages/core/test/render3/listeners_spec.ts b/packages/core/test/render3/listeners_spec.ts index 06d10fd1ed..f35e593a5a 100644 --- a/packages/core/test/render3/listeners_spec.ts +++ b/packages/core/test/render3/listeners_spec.ts @@ -6,12 +6,13 @@ * found in the LICENSE file at https://angular.io/license */ -import {bind, defineComponent, defineDirective, markDirty, textBinding} from '../../src/render3/index'; +import {bind, defineComponent, defineDirective, markDirty, reference, textBinding} from '../../src/render3/index'; import {container, containerRefreshEnd, containerRefreshStart, element, elementEnd, elementStart, embeddedViewEnd, embeddedViewStart, listener, text} from '../../src/render3/instructions'; import {RenderFlags} from '../../src/render3/interfaces/definition'; +import {getCurrentView, restoreView} from '../../src/render3/state'; import {getRendererFactory2} from './imported_renderer2'; -import {ComponentFixture, containerEl, renderToHtml, requestAnimationFrame} from './render_util'; +import {ComponentFixture, containerEl, createComponent, getDirectiveOnNode, renderToHtml, requestAnimationFrame} from './render_util'; describe('event listeners', () => { @@ -723,4 +724,54 @@ describe('event listeners', () => { }); + it('should support local refs in listeners', () => { + let compInstance: any; + + const Comp = createComponent('comp', (rf: RenderFlags, ctx: any) => {}); + + /** + * + * + */ + class App { + comp: any = null; + + onClick(comp: any) { this.comp = comp; } + + static ngComponentDef = defineComponent({ + type: App, + selectors: [['app']], + factory: () => new App(), + consts: 3, + vars: 0, + template: (rf: RenderFlags, ctx: App) => { + if (rf & RenderFlags.Create) { + const state = getCurrentView(); + element(0, 'comp', null, ['comp', '']); + elementStart(2, 'button'); + { + listener('click', function() { + restoreView(state); + const comp = reference(1); + return ctx.onClick(comp); + }); + } + elementEnd(); + } + + // testing only + compInstance = getDirectiveOnNode(0); + }, + directives: [Comp] + }); + } + + const fixture = new ComponentFixture(App); + expect(fixture.component.comp).toEqual(null); + + const button = fixture.hostElement.querySelector('button') as HTMLButtonElement; + button.click(); + expect(fixture.component.comp).toEqual(compInstance); + }); + }); diff --git a/packages/forms/test/template_integration_spec.ts b/packages/forms/test/template_integration_spec.ts index f000b718ab..7d2f8bbb18 100644 --- a/packages/forms/test/template_integration_spec.ts +++ b/packages/forms/test/template_integration_spec.ts @@ -1489,36 +1489,35 @@ import {NgModelCustomComp, NgModelCustomWrapper} from './value_accessor_integrat expect(required.nativeElement.getAttribute('pattern')).toEqual(null); })); - fixmeIvy('ngModelChange callback never called') && - it('should update control status', fakeAsync(() => { - const fixture = initTest(NgModelChangeState); - const inputEl = fixture.debugElement.query(By.css('input')); - const inputNativeEl = inputEl.nativeElement; - const onNgModelChange = jasmine.createSpy('onNgModelChange'); - fixture.componentInstance.onNgModelChange = onNgModelChange; - fixture.detectChanges(); - tick(); + it('should update control status', fakeAsync(() => { + const fixture = initTest(NgModelChangeState); + const inputEl = fixture.debugElement.query(By.css('input')); + const inputNativeEl = inputEl.nativeElement; + const onNgModelChange = jasmine.createSpy('onNgModelChange'); + fixture.componentInstance.onNgModelChange = onNgModelChange; + fixture.detectChanges(); + tick(); - expect(onNgModelChange).not.toHaveBeenCalled(); + expect(onNgModelChange).not.toHaveBeenCalled(); - inputNativeEl.value = 'updated'; - onNgModelChange.and.callFake((ngModel: NgModel) => { - expect(ngModel.invalid).toBe(true); - expect(ngModel.value).toBe('updated'); - }); - dispatchEvent(inputNativeEl, 'input'); - expect(onNgModelChange).toHaveBeenCalled(); - tick(); + inputNativeEl.value = 'updated'; + onNgModelChange.and.callFake((ngModel: NgModel) => { + expect(ngModel.invalid).toBe(true); + expect(ngModel.value).toBe('updated'); + }); + dispatchEvent(inputNativeEl, 'input'); + expect(onNgModelChange).toHaveBeenCalled(); + tick(); - inputNativeEl.value = '333'; - onNgModelChange.and.callFake((ngModel: NgModel) => { - expect(ngModel.invalid).toBe(false); - expect(ngModel.value).toBe('333'); - }); - dispatchEvent(inputNativeEl, 'input'); - expect(onNgModelChange).toHaveBeenCalledTimes(2); - tick(); - })); + inputNativeEl.value = '333'; + onNgModelChange.and.callFake((ngModel: NgModel) => { + expect(ngModel.invalid).toBe(false); + expect(ngModel.value).toBe('333'); + }); + dispatchEvent(inputNativeEl, 'input'); + expect(onNgModelChange).toHaveBeenCalledTimes(2); + tick(); + })); });