fix(core): only apply WrappedValue to the binding of the pipe (#15257)

Previously, a pipe that returned a `WrappedValue` would force the change
of the next bound property, independent of the binding in which the pipe
was used.

Now only the binding in which the `WrappedValue` is used will be assumed
as changed.

Fixes #15116

PR Close #15257

packages/compiler/src/view_compiler/view_compiler.ts

@@ -89,6 +89,8 @@ interface ViewBuilderFactory {
 
 interface UpdateExpression {
   context: o.Expression;
+  nodeIndex: number;
+  bindingIndex: number;
   sourceSpan: ParseSourceSpan;
   value: AST;
 }
@@ -253,8 +255,8 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver {
     const inter = <Interpolation>astWithSource.ast;
 
     const updateRendererExpressions = inter.expressions.map(
-        (expr) => this._preprocessUpdateExpression(
+        (expr, bindingIndex) => this._preprocessUpdateExpression(
-            {sourceSpan: ast.sourceSpan, context: COMP_VAR, value: expr}));
+            {nodeIndex, bindingIndex, sourceSpan: ast.sourceSpan, context: COMP_VAR, value: expr}));
 
     // textDef(ngContentIndex: number, constants: string[]): NodeDef;
     this.nodes[nodeIndex] = () => ({
@@ -323,8 +325,10 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver {
                                .concat(dirHostBindings);
       if (hostBindings.length) {
         updateRendererExpressions =
-            hostBindings.map((hostBinding) => this._preprocessUpdateExpression({
+            hostBindings.map((hostBinding, bindingIndex) => this._preprocessUpdateExpression({
               context: hostBinding.context,
+              nodeIndex,
+              bindingIndex,
               sourceSpan: hostBinding.inputAst.sourceSpan,
               value: hostBinding.inputAst.value
             }));
@@ -545,9 +549,14 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver {
     });
     let updateDirectiveExpressions: UpdateExpression[] = [];
     if (dirAst.inputs.length || (flags & (NodeFlags.DoCheck | NodeFlags.OnInit)) > 0) {
-      updateDirectiveExpressions = dirAst.inputs.map(
+      updateDirectiveExpressions =
-          (input) => this._preprocessUpdateExpression(
+          dirAst.inputs.map((input, bindingIndex) => this._preprocessUpdateExpression({
-              {sourceSpan: input.sourceSpan, context: COMP_VAR, value: input.value}));
+            nodeIndex,
+            bindingIndex,
+            sourceSpan: input.sourceSpan,
+            context: COMP_VAR,
+            value: input.value
+          }));
     }
 
     const dirContextExpr = o.importExpr(createIdentifier(Identifiers.nodeValue)).callFn([
@@ -694,14 +703,14 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver {
 
     return (args: o.Expression[]) => callCheckStmt(nodeIndex, args);
   }
-  createPipeConverter(sourceSpan: ParseSourceSpan, name: string, argCount: number):
+  createPipeConverter(expression: UpdateExpression, name: string, argCount: number):
       BuiltinConverter {
     const pipe = this.usedPipes.find((pipeSummary) => pipeSummary.name === name);
     if (pipe.pure) {
       const nodeIndex = this.nodes.length;
       // function purePipeDef(argCount: number): NodeDef;
       this.nodes.push(() => ({
-                        sourceSpan,
+                        sourceSpan: expression.sourceSpan,
                         nodeDef: o.importExpr(createIdentifier(Identifiers.purePipeDef))
                                      .callFn([o.literal(argCount)])
                       }));
@@ -719,15 +728,18 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver {
             compViewExpr, o.literal(pipeNodeIndex)
           ]);
 
-      return (args: o.Expression[]) =>
+      return (args: o.Expression[]) => callUnwrapValue(
-                 callUnwrapValue(callCheckStmt(nodeIndex, [pipeValueExpr].concat(args)));
+                 expression.nodeIndex, expression.bindingIndex,
+                 callCheckStmt(nodeIndex, [pipeValueExpr].concat(args)));
     } else {
-      const nodeIndex = this._createPipe(sourceSpan, pipe);
+      const nodeIndex = this._createPipe(expression.sourceSpan, pipe);
       const nodeValueExpr = o.importExpr(createIdentifier(Identifiers.nodeValue)).callFn([
         VIEW_VAR, o.literal(nodeIndex)
       ]);
 
-      return (args: o.Expression[]) => callUnwrapValue(nodeValueExpr.callMethod('transform', args));
+      return (args: o.Expression[]) => callUnwrapValue(
+                 expression.nodeIndex, expression.bindingIndex,
+                 nodeValueExpr.callMethod('transform', args));
     }
   }
 
@@ -756,6 +768,8 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver {
   // Attention: This might create new nodeDefs (for pipes and literal arrays and literal maps)!
   private _preprocessUpdateExpression(expression: UpdateExpression): UpdateExpression {
     return {
+      nodeIndex: expression.nodeIndex,
+      bindingIndex: expression.bindingIndex,
       sourceSpan: expression.sourceSpan,
       context: expression.context,
       value: convertPropertyBindingBuiltins(
@@ -765,7 +779,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver {
             createLiteralMapConverter:
                 (keys: string[]) => this.createLiteralMapConverter(expression.sourceSpan, keys),
             createPipeConverter: (name: string, argCount: number) =>
-                                     this.createPipeConverter(expression.sourceSpan, name, argCount)
+                                     this.createPipeConverter(expression, name, argCount)
           },
           expression.value)
     };
@@ -1070,8 +1084,10 @@ function callCheckStmt(nodeIndex: number, exprs: o.Expression[]): o.Expression {
   }
 }
 
-function callUnwrapValue(expr: o.Expression): o.Expression {
+function callUnwrapValue(nodeIndex: number, bindingIdx: number, expr: o.Expression): o.Expression {
-  return o.importExpr(createIdentifier(Identifiers.unwrapValue)).callFn([expr]);
+  return o.importExpr(createIdentifier(Identifiers.unwrapValue)).callFn([
+    VIEW_VAR, o.literal(nodeIndex), o.literal(bindingIdx), expr
+  ]);
 }
 
 interface StaticAndDynamicQueryIds {

packages/core/src/view/provider.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ChangeDetectorRef, SimpleChange, SimpleChanges} from '../change_detection/change_detection';
+import {ChangeDetectorRef, SimpleChange, SimpleChanges, WrappedValue} from '../change_detection/change_detection';
 import {Injector} from '../di';
 import {ElementRef} from '../linker/element_ref';
 import {TemplateRef} from '../linker/template_ref';
@@ -450,7 +450,10 @@ function updateProp(
   providerData.instance[propName] = value;
   if (def.flags & NodeFlags.OnChanges) {
     changes = changes || {};
-    const oldValue = view.oldValues[def.bindingIndex + bindingIdx];
+    let oldValue = view.oldValues[def.bindingIndex + bindingIdx];
+    if (oldValue instanceof WrappedValue) {
+      oldValue = oldValue.wrapped;
+    }
     const binding = def.bindings[bindingIdx];
     changes[binding.nonMinifiedName] =
         new SimpleChange(oldValue, value, (view.state & ViewState.FirstCheck) !== 0);

packages/core/src/view/util.ts

@@ -32,12 +32,15 @@ export function tokenKey(token: any): string {
   return key;
 }
 
-let unwrapCounter = 0;
+export function unwrapValue(view: ViewData, nodeIdx: number, bindingIdx: number, value: any): any {
-
-export function unwrapValue(value: any): any {
   if (value instanceof WrappedValue) {
     value = value.wrapped;
-    unwrapCounter++;
+    let globalBindingIdx = view.def.nodes[nodeIdx].bindingIndex + bindingIdx;
+    let oldValue = view.oldValues[globalBindingIdx];
+    if (oldValue instanceof WrappedValue) {
+      oldValue = oldValue.wrapped;
+    }
+    view.oldValues[globalBindingIdx] = new WrappedValue(oldValue);
   }
   return value;
 }
@@ -83,9 +86,8 @@ export function resolveRendererType2(type: RendererType2): RendererType2 {
 export function checkBinding(
     view: ViewData, def: NodeDef, bindingIdx: number, value: any): boolean {
   const oldValues = view.oldValues;
-  if (unwrapCounter > 0 || !!(view.state & ViewState.FirstCheck) ||
+  if ((view.state & ViewState.FirstCheck) ||
       !looseIdentical(oldValues[def.bindingIndex + bindingIdx], value)) {
-    unwrapCounter = 0;
     return true;
   }
   return false;
@@ -103,8 +105,7 @@ export function checkAndUpdateBinding(
 export function checkBindingNoChanges(
     view: ViewData, def: NodeDef, bindingIdx: number, value: any) {
   const oldValue = view.oldValues[def.bindingIndex + bindingIdx];
-  if (unwrapCounter || (view.state & ViewState.FirstCheck) || !devModeEqual(oldValue, value)) {
+  if ((view.state & ViewState.FirstCheck) || !devModeEqual(oldValue, value)) {
-    unwrapCounter = 0;
     throw expressionChangedAfterItHasBeenCheckedError(
         Services.createDebugContext(view, def.index), oldValue, value,
         (view.state & ViewState.FirstCheck) !== 0);

packages/core/test/linker/change_detection_integration_spec.ts

@@ -515,7 +515,7 @@ export function main() {
              expect(renderLog.log).toEqual([]);
            }));
 
-        it('should unwrap the wrapped value', fakeAsync(() => {
+        it('should unwrap the wrapped value and force a change', fakeAsync(() => {
              const ctx = _bindSimpleValue('"Megatron" | wrappedPipe', Person);
 
              ctx.detectChanges(false);
@@ -528,6 +528,28 @@ export function main() {
              expect(renderLog.log).toEqual(['someProp=Megatron']);
            }));
 
+        it('should record unwrapped values via ngOnChanges', fakeAsync(() => {
+             const ctx = createCompFixture(
+                 '<div [testDirective]="\'aName\' | wrappedPipe" [a]="1" [b]="2 | wrappedPipe"></div>');
+             const dir: TestDirective = queryDirs(ctx.debugElement, TestDirective)[0];
+             ctx.detectChanges(false);
+             dir.changes = {};
+             ctx.detectChanges(false);
+
+             // Note: the binding for `b` did not change and has no ValueWrapper,
+             // and should therefore stay unchanged.
+             expect(dir.changes).toEqual({
+               'name': new SimpleChange('aName', 'aName', false),
+               'b': new SimpleChange(2, 2, false)
+             });
+
+             ctx.detectChanges(false);
+             expect(dir.changes).toEqual({
+               'name': new SimpleChange('aName', 'aName', false),
+               'b': new SimpleChange(2, 2, false)
+             });
+           }));
+
         it('should call pure pipes only if the arguments change', fakeAsync(() => {
              const ctx = _bindSimpleValue('name | countingPipe', Person);
              // change from undefined -> null
@@ -669,9 +691,16 @@ export function main() {
                  '<div [testDirective]="\'aName\'" [a]="1" [b]="2"></div><div [testDirective]="\'bName\'" [a]="4"></div>');
              ctx.detectChanges(false);
 
-             const dirs = queryDirs(ctx.debugElement, TestDirective);
+             const dirs = <TestDirective[]>queryDirs(ctx.debugElement, TestDirective);
-             expect(dirs[0].changes).toEqual({'a': 1, 'b': 2, 'name': 'aName'});
+             expect(dirs[0].changes).toEqual({
-             expect(dirs[1].changes).toEqual({'a': 4, 'name': 'bName'});
+               'a': new SimpleChange(undefined, 1, true),
+               'b': new SimpleChange(undefined, 2, true),
+               'name': new SimpleChange(undefined, 'aName', true)
+             });
+             expect(dirs[1].changes).toEqual({
+               'a': new SimpleChange(undefined, 4, true),
+               'name': new SimpleChange(undefined, 'bName', true)
+             });
            }));
       });
     });
@@ -1457,7 +1486,7 @@ class TestDirective implements OnInit, DoCheck, OnChanges, AfterContentInit, Aft
     AfterViewInit, AfterViewChecked, OnDestroy {
   @Input() a: any;
   @Input() b: any;
-  changes: any;
+  changes: SimpleChanges;
   event: any;
   eventEmitter: EventEmitter<string> = new EventEmitter<string>();
 
@@ -1480,9 +1509,7 @@ class TestDirective implements OnInit, DoCheck, OnChanges, AfterContentInit, Aft
 
   ngOnChanges(changes: SimpleChanges) {
     this.log.add(this.name, 'ngOnChanges');
-    const r: {[k: string]: string} = {};
+    this.changes = changes;
-    Object.keys(changes).forEach(key => { r[key] = changes[key].currentValue; });
-    this.changes = r;
     if (this.throwOn == 'ngOnChanges') {
       throw new Error('Boom!');
     }

packages/core/test/linker/regression_integration_spec.ts

@@ -6,8 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ANALYZE_FOR_ENTRY_COMPONENTS, Component, Directive, InjectionToken, Injector, Input, Pipe, PipeTransform, Provider, QueryList, Renderer2, TemplateRef, ViewChildren, ViewContainerRef} from '@angular/core';
+import {ANALYZE_FOR_ENTRY_COMPONENTS, Component, Directive, InjectionToken, Injector, Input, Pipe, PipeTransform, Provider, QueryList, Renderer2, SimpleChanges, TemplateRef, ViewChildren, ViewContainerRef} from '@angular/core';
-import {TestBed} from '@angular/core/testing';
+import {TestBed, fakeAsync, tick} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
 
@@ -68,6 +68,43 @@ function declareTests({useJit}: {useJit: boolean}) {
         expect(CountingPipe.calls).toBe(1);
       });
 
+      it('should only update the bound property when using asyncPipe - #15205', fakeAsync(() => {
+           @Component({template: '<div myDir [a]="p | async" [b]="2"></div>'})
+           class MyComp {
+             p = Promise.resolve(1);
+           }
+
+           @Directive({selector: '[myDir]'})
+           class MyDir {
+             setterCalls: {[key: string]: any} = {};
+             changes: SimpleChanges;
+
+             @Input()
+             set a(v: number) { this.setterCalls['a'] = v; }
+             @Input()
+             set b(v: number) { this.setterCalls['b'] = v; }
+
+             ngOnChanges(changes: SimpleChanges) { this.changes = changes; }
+           }
+
+           TestBed.configureTestingModule({declarations: [MyDir, MyComp]});
+           const fixture = TestBed.createComponent(MyComp);
+           const dir = fixture.debugElement.query(By.directive(MyDir)).injector.get(MyDir) as MyDir;
+
+           fixture.detectChanges();
+           expect(dir.setterCalls).toEqual({'a': null, 'b': 2});
+           expect(Object.keys(dir.changes)).toEqual(['a', 'b']);
+
+           dir.setterCalls = {};
+           dir.changes = {};
+
+           tick();
+           fixture.detectChanges();
+
+           expect(dir.setterCalls).toEqual({'a': 1});
+           expect(Object.keys(dir.changes)).toEqual(['a']);
+         }));
+
       it('should only evaluate methods once - #10639', () => {
         TestBed.configureTestingModule({declarations: [MyCountingComp]});
         const template = '{{method()?.value}}';