diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts index 59b857e7ba..79cb9211be 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts @@ -180,6 +180,44 @@ describe('compiler compliance: bindings', () => { expectEmit(result.source, template, 'Incorrect template'); }); + it('should emit temporary evaluation within the binding expression for in-order execution', + () => { + // https://github.com/angular/angular/issues/37194 + // Verifies that temporary expressions used for expressions with potential side-effects in + // the LHS of a safe navigation access are emitted within the binding expression itself, to + // ensure that these temporaries are evaluated during the evaluation of the binding. This + // is important for when the LHS contains a pipe, as pipe evaluation depends on the current + // binding index. + const files = { + app: { + 'example.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: '' + }) + export class MyComponent { + myTitle = 'hello'; + auth?: () => { identity(): any; }; + }` + } + }; + + const result = compile(files, angularFiles); + const template = ` + … + template: function MyComponent_Template(rf, ctx) { + … + if (rf & 2) { + var $tmp0$ = null; + $r3$.ɵɵproperty("title", ctx.myTitle)("id", ($tmp0$ = $r3$.ɵɵpipeBind1(1, 3, ($tmp0$ = ctx.auth()) == null ? null : $tmp0$.identity())) == null ? null : $tmp0$.id)("tabindex", 1); + } + } + `; + + expectEmit(result.source, template, 'Incorrect template'); + }); + it('should chain multiple property bindings into a single instruction', () => { const files = { app: { @@ -685,6 +723,46 @@ describe('compiler compliance: bindings', () => { expectEmit(source, HostBindingDirDeclaration, 'Invalid host binding code'); }); + it('should support host bindings with temporary expressions', () => { + const files = { + app: { + 'spec.ts': ` + import {Directive, NgModule} from '@angular/core'; + + @Directive({ + selector: '[hostBindingDir]', + host: {'[id]': 'getData()?.id'} + }) + export class HostBindingDir { + getData?: () => { id: number }; + } + + @NgModule({declarations: [HostBindingDir]}) + export class MyModule {} + ` + } + }; + + const HostBindingDirDeclaration = ` + HostBindingDir.ɵdir = $r3$.ɵɵdefineDirective({ + type: HostBindingDir, + selectors: [["", "hostBindingDir", ""]], + hostVars: 1, + hostBindings: function HostBindingDir_HostBindings(rf, ctx) { + if (rf & 2) { + var $tmp0$ = null; + $r3$.ɵɵhostProperty("id", ($tmp0$ = ctx.getData()) == null ? null : $tmp0$.id); + } + } + }); + `; + + const result = compile(files, angularFiles); + const source = result.source; + + expectEmit(source, HostBindingDirDeclaration, 'Invalid host binding code'); + }); + it('should support host bindings with pure functions', () => { const files = { app: { diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts index 2755e0b57a..533046c8ef 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts @@ -805,8 +805,7 @@ describe('i18n support in the template compiler', () => { } if (rf & 2) { var $tmp_0_0$ = null; - const $currVal_0$ = ($tmp_0_0$ = ctx.valueA.getRawValue()) == null ? null : $tmp_0_0$.getTitle(); - $r3$.ɵɵi18nExp($currVal_0$); + $r3$.ɵɵi18nExp(($tmp_0_0$ = ctx.valueA.getRawValue()) == null ? null : $tmp_0_0$.getTitle()); $r3$.ɵɵi18nApply(1); } } @@ -1320,9 +1319,8 @@ describe('i18n support in the template compiler', () => { } if (rf & 2) { var $tmp_2_0$ = null; - const $currVal_2$ = ($tmp_2_0$ = ctx.valueA.getRawValue()) == null ? null : $tmp_2_0$.getTitle(); $r3$.ɵɵadvance(2); - $r3$.ɵɵi18nExp($r3$.ɵɵpipeBind1(2, 3, ctx.valueA))(ctx.valueA == null ? null : ctx.valueA.a == null ? null : ctx.valueA.a.b)($currVal_2$); + $r3$.ɵɵi18nExp($r3$.ɵɵpipeBind1(2, 3, ctx.valueA))(ctx.valueA == null ? null : ctx.valueA.a == null ? null : ctx.valueA.a.b)(($tmp_2_0$ = ctx.valueA.getRawValue()) == null ? null : $tmp_2_0$.getTitle()); $r3$.ɵɵi18nApply(1); } } diff --git a/packages/compiler/src/compiler_util/expression_converter.ts b/packages/compiler/src/compiler_util/expression_converter.ts index 1abb1c39a5..240c35b66f 100644 --- a/packages/compiler/src/compiler_util/expression_converter.ts +++ b/packages/compiler/src/compiler_util/expression_converter.ts @@ -154,6 +154,11 @@ export enum BindingForm { // Try to generate a simple binding (no temporaries or statements) // otherwise generate a general binding TrySimple, + + // Inlines assignment of temporaries into the generated expression. The result may still + // have statements attached for declarations of temporary variables. + // This is the only relevant form for Ivy, the other forms are only used in ViewEngine. + Expression, } /** @@ -168,7 +173,6 @@ export function convertPropertyBinding( if (!localResolver) { localResolver = new DefaultLocalResolver(); } - const currValExpr = createCurrValueExpr(bindingId); const visitor = new _AstToIrVisitor(localResolver, implicitReceiver, bindingId, interpolationFunction); const outputExpr: o.Expression = expressionWithoutBuiltins.visit(visitor, _Mode.Expression); @@ -180,8 +184,11 @@ export function convertPropertyBinding( if (visitor.temporaryCount === 0 && form == BindingForm.TrySimple) { return new ConvertPropertyBindingResult([], outputExpr); + } else if (form === BindingForm.Expression) { + return new ConvertPropertyBindingResult(stmts, outputExpr); } + const currValExpr = createCurrValueExpr(bindingId); stmts.push(currValExpr.set(outputExpr).toDeclStmt(o.DYNAMIC_TYPE, [o.StmtModifier.Final])); return new ConvertPropertyBindingResult(stmts, currValExpr); } diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index 7adf55fb43..2aaeddc1fe 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -714,7 +714,7 @@ function createHostBindingsFunction( function bindingFn(implicit: any, value: AST) { return convertPropertyBinding( - null, implicit, value, 'b', BindingForm.TrySimple, () => error('Unexpected interpolation')); + null, implicit, value, 'b', BindingForm.Expression, () => error('Unexpected interpolation')); } function convertStylingCall( diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 4e12be2daf..4cb1601915 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -1213,7 +1213,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver private convertPropertyBinding(value: AST): o.Expression { const convertedPropertyBinding = convertPropertyBinding( - this, this.getImplicitReceiverExpr(), value, this.bindingContext(), BindingForm.TrySimple, + this, this.getImplicitReceiverExpr(), value, this.bindingContext(), BindingForm.Expression, () => error('Unexpected interpolation')); const valExpr = convertedPropertyBinding.currValExpr; this._tempVariables.push(...convertedPropertyBinding.stmts); diff --git a/packages/core/test/acceptance/pipe_spec.ts b/packages/core/test/acceptance/pipe_spec.ts index 2befc8e240..22a38f7729 100644 --- a/packages/core/test/acceptance/pipe_spec.ts +++ b/packages/core/test/acceptance/pipe_spec.ts @@ -6,9 +6,10 @@ * found in the LICENSE file at https://angular.io/license */ -import {ChangeDetectionStrategy, ChangeDetectorRef, Component, Directive, Inject, Injectable, InjectionToken, Input, NgModule, OnDestroy, Pipe, PipeTransform, ViewChild} from '@angular/core'; +import {ChangeDetectionStrategy, ChangeDetectorRef, Component, Directive, Inject, Injectable, InjectionToken, Input, NgModule, OnChanges, OnDestroy, Pipe, PipeTransform, SimpleChanges, ViewChild, WrappedValue} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; +import {ivyEnabled} from '@angular/private/testing'; describe('pipe', () => { @Pipe({name: 'countingPipe'}) @@ -285,6 +286,133 @@ describe('pipe', () => { expect(fixture.nativeElement).toHaveText('a'); }); + describe('pipes within an optional chain', () => { + it('should not dirty unrelated inputs', () => { + // https://github.com/angular/angular/issues/37194 + // https://github.com/angular/angular/issues/37591 + // Using a pipe in the LHS of safe navigation operators would clobber unrelated bindings + // iff the pipe returns WrappedValue, incorrectly causing the unrelated binding + // to be considered changed. + const log: string[] = []; + + @Component({template: ``}) + class App { + value2 = {id: 2}; + } + + @Component({selector: 'my-cmp', template: ''}) + class MyCmp { + @Input() + set value1(value1: number) { + log.push(`set value1=${value1}`); + } + + @Input() + set value2(value2: number) { + log.push(`set value2=${value2}`); + } + } + + @Pipe({name: 'pipe'}) + class MyPipe implements PipeTransform { + transform(value: any): any { + log.push('pipe'); + return WrappedValue.wrap(value); + } + } + + TestBed.configureTestingModule({declarations: [App, MyCmp, MyPipe]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(/* checkNoChanges */ false); + + // Both bindings should have been set. Note: ViewEngine evaluates the pipe out-of-order, + // before setting inputs. + expect(log).toEqual( + ivyEnabled ? + [ + 'set value1=1', + 'pipe', + 'set value2=2', + ] : + [ + 'pipe', + 'set value1=1', + 'set value2=2', + ]); + log.length = 0; + + fixture.componentInstance.value2 = {id: 3}; + fixture.detectChanges(/* checkNoChanges */ false); + + // value1 did not change, so it should not have been set. + expect(log).toEqual([ + 'pipe', + 'set value2=3', + ]); + }); + + it('should not include unrelated inputs in ngOnChanges', () => { + // https://github.com/angular/angular/issues/37194 + // https://github.com/angular/angular/issues/37591 + // Using a pipe in the LHS of safe navigation operators would clobber unrelated bindings + // iff the pipe returns WrappedValue, incorrectly causing the unrelated binding + // to be considered changed. + const log: string[] = []; + + @Component({template: ``}) + class App { + value2 = {id: 2}; + } + + @Component({selector: 'my-cmp', template: ''}) + class MyCmp implements OnChanges { + @Input() value1!: number; + + @Input() value2!: number; + + ngOnChanges(changes: SimpleChanges): void { + if (changes.value1) { + const {previousValue, currentValue, firstChange} = changes.value1; + log.push(`change value1: ${previousValue} -> ${currentValue} (${firstChange})`); + } + if (changes.value2) { + const {previousValue, currentValue, firstChange} = changes.value2; + log.push(`change value2: ${previousValue} -> ${currentValue} (${firstChange})`); + } + } + } + + @Pipe({name: 'pipe'}) + class MyPipe implements PipeTransform { + transform(value: any): any { + log.push('pipe'); + return WrappedValue.wrap(value); + } + } + + TestBed.configureTestingModule({declarations: [App, MyCmp, MyPipe]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(/* checkNoChanges */ false); + + // Both bindings should have been included in ngOnChanges. + expect(log).toEqual([ + 'pipe', + 'change value1: undefined -> 1 (true)', + 'change value2: undefined -> 2 (true)', + ]); + log.length = 0; + + fixture.componentInstance.value2 = {id: 3}; + fixture.detectChanges(/* checkNoChanges */ false); + + // value1 did not change, so it should not have been included in ngOnChanges + expect(log).toEqual([ + 'pipe', + 'change value2: 2 -> 3 (false)', + ]); + }); + }); + describe('pure', () => { it('should call pure pipes only if the arguments change', () => { @Component({