diff --git a/packages/compiler/src/view_compiler/view_compiler.ts b/packages/compiler/src/view_compiler/view_compiler.ts index be445c5b48..1a8dd29807 100644 --- a/packages/compiler/src/view_compiler/view_compiler.ts +++ b/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 = astWithSource.ast; const updateRendererExpressions = inter.expressions.map( - (expr) => this._preprocessUpdateExpression( - {sourceSpan: ast.sourceSpan, context: COMP_VAR, value: expr})); + (expr, bindingIndex) => this._preprocessUpdateExpression( + {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( - (input) => this._preprocessUpdateExpression( - {sourceSpan: input.sourceSpan, context: COMP_VAR, value: input.value})); + updateDirectiveExpressions = + dirAst.inputs.map((input, bindingIndex) => this._preprocessUpdateExpression({ + 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[]) => - callUnwrapValue(callCheckStmt(nodeIndex, [pipeValueExpr].concat(args))); + return (args: o.Expression[]) => callUnwrapValue( + 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 { - return o.importExpr(createIdentifier(Identifiers.unwrapValue)).callFn([expr]); +function callUnwrapValue(nodeIndex: number, bindingIdx: number, expr: o.Expression): o.Expression { + return o.importExpr(createIdentifier(Identifiers.unwrapValue)).callFn([ + VIEW_VAR, o.literal(nodeIndex), o.literal(bindingIdx), expr + ]); } interface StaticAndDynamicQueryIds { diff --git a/packages/core/src/view/provider.ts b/packages/core/src/view/provider.ts index 0d728af7ab..2c0d8adfd6 100644 --- a/packages/core/src/view/provider.ts +++ b/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); diff --git a/packages/core/src/view/util.ts b/packages/core/src/view/util.ts index 2e41682500..18f846eaa8 100644 --- a/packages/core/src/view/util.ts +++ b/packages/core/src/view/util.ts @@ -32,12 +32,15 @@ export function tokenKey(token: any): string { return key; } -let unwrapCounter = 0; - -export function unwrapValue(value: any): any { +export function unwrapValue(view: ViewData, nodeIdx: number, bindingIdx: number, 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)) { - unwrapCounter = 0; + if ((view.state & ViewState.FirstCheck) || !devModeEqual(oldValue, value)) { throw expressionChangedAfterItHasBeenCheckedError( Services.createDebugContext(view, def.index), oldValue, value, (view.state & ViewState.FirstCheck) !== 0); diff --git a/packages/core/test/linker/change_detection_integration_spec.ts b/packages/core/test/linker/change_detection_integration_spec.ts index 049838cc9e..03706e3e09 100644 --- a/packages/core/test/linker/change_detection_integration_spec.ts +++ b/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( + '
'); + 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() { '
'); ctx.detectChanges(false); - const dirs = queryDirs(ctx.debugElement, TestDirective); - expect(dirs[0].changes).toEqual({'a': 1, 'b': 2, 'name': 'aName'}); - expect(dirs[1].changes).toEqual({'a': 4, 'name': 'bName'}); + const dirs = queryDirs(ctx.debugElement, TestDirective); + expect(dirs[0].changes).toEqual({ + '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 = new EventEmitter(); @@ -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} = {}; - Object.keys(changes).forEach(key => { r[key] = changes[key].currentValue; }); - this.changes = r; + this.changes = changes; if (this.throwOn == 'ngOnChanges') { throw new Error('Boom!'); } diff --git a/packages/core/test/linker/regression_integration_spec.ts b/packages/core/test/linker/regression_integration_spec.ts index 3c8bc89bff..cf571c4924 100644 --- a/packages/core/test/linker/regression_integration_spec.ts +++ b/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 {TestBed} from '@angular/core/testing'; +import {ANALYZE_FOR_ENTRY_COMPONENTS, Component, Directive, InjectionToken, Injector, Input, Pipe, PipeTransform, Provider, QueryList, Renderer2, SimpleChanges, TemplateRef, ViewChildren, ViewContainerRef} from '@angular/core'; +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: '
'}) + 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}}';