From dd6e0cf1b5b658f54e0b490457b3d2ade5a1cc74 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Tue, 3 May 2016 08:59:25 -0700 Subject: [PATCH] fix(compiler): fix where pipes live Impure pipes need to live on the view that used them and need a new instance for each call site. Impure pipes need to live on the component view, cached across all child views, and need a new pure proxy for each for each call site that lives on the view of the call site. Fixes #8408 This bug was introduced not long ago by 152a117d5c27e56d1b32d69df2f69d34b94c0760 --- .../src/view_compiler/compile_pipe.ts | 52 +++++++++++++------ .../src/view_compiler/compile_view.ts | 11 +--- .../change_detection_integration_spec.ts | 17 ++++-- .../linker/view_injector_integration_spec.ts | 27 +++++++--- 4 files changed, 71 insertions(+), 36 deletions(-) diff --git a/modules/@angular/compiler/src/view_compiler/compile_pipe.ts b/modules/@angular/compiler/src/view_compiler/compile_pipe.ts index 9550d7257e..12a274d3e8 100644 --- a/modules/@angular/compiler/src/view_compiler/compile_pipe.ts +++ b/modules/@angular/compiler/src/view_compiler/compile_pipe.ts @@ -7,17 +7,35 @@ import {Identifiers, identifierToken} from '../identifiers'; import {injectFromViewParentInjector, createPureProxy, getPropertyInView} from './util'; class _PurePipeProxy { - constructor(public instance: o.ReadPropExpr, public argCount: number) {} + constructor(public view: CompileView, public instance: o.ReadPropExpr, public argCount: number) {} } export class CompilePipe { - meta: CompilePipeMetadata; + static call(view: CompileView, name: string, args: o.Expression[]): o.Expression { + var compView = view.componentView; + var meta = _findPipeMeta(compView, name); + var pipe: CompilePipe; + if (meta.pure) { + // pure pipes live on the component view + pipe = compView.purePipes.get(name); + if (isBlank(pipe)) { + pipe = new CompilePipe(compView, meta); + compView.purePipes.set(name, pipe); + compView.pipes.push(pipe); + } + } else { + // Non pure pipes live on the view that called it + pipe = new CompilePipe(view, meta); + view.pipes.push(pipe); + } + return pipe._call(view, args); + } + instance: o.ReadPropExpr; private _purePipeProxies: _PurePipeProxy[] = []; - constructor(public view: CompileView, name: string) { - this.meta = _findPipeMeta(view, name); - this.instance = o.THIS_EXPR.prop(`_pipe_${name}_${view.pipeCount++}`); + constructor(public view: CompileView, public meta: CompilePipeMetadata) { + this.instance = o.THIS_EXPR.prop(`_pipe_${meta.name}_${view.pipeCount++}`); } get pure(): boolean { return this.meta.pure; } @@ -35,21 +53,26 @@ export class CompilePipe { .set(o.importExpr(this.meta.type).instantiate(deps)) .toStmt()); this._purePipeProxies.forEach((purePipeProxy) => { - createPureProxy( - this.instance.prop('transform').callMethod(o.BuiltinMethod.bind, [this.instance]), - purePipeProxy.argCount, purePipeProxy.instance, this.view); + var pipeInstanceSeenFromPureProxy = + getPropertyInView(this.instance, purePipeProxy.view, this.view); + createPureProxy(pipeInstanceSeenFromPureProxy.prop('transform') + .callMethod(o.BuiltinMethod.bind, [pipeInstanceSeenFromPureProxy]), + purePipeProxy.argCount, purePipeProxy.instance, purePipeProxy.view); }); } - call(callingView: CompileView, args: o.Expression[]): o.Expression { + private _call(callingView: CompileView, args: o.Expression[]): o.Expression { if (this.meta.pure) { + // PurePipeProxies live on the view that called them. var purePipeProxy = new _PurePipeProxy( - o.THIS_EXPR.prop(`${this.instance.name}_${this._purePipeProxies.length}`), args.length); + callingView, o.THIS_EXPR.prop(`${this.instance.name}_${this._purePipeProxies.length}`), + args.length); this._purePipeProxies.push(purePipeProxy); - return getPropertyInView( - o.importExpr(Identifiers.castByValue) - .callFn([purePipeProxy.instance, this.instance.prop('transform')]), - callingView, this.view) + return o.importExpr(Identifiers.castByValue) + .callFn([ + purePipeProxy.instance, + getPropertyInView(this.instance.prop('transform'), callingView, this.view) + ]) .callFn(args); } else { return getPropertyInView(this.instance, callingView, this.view).callMethod('transform', args); @@ -57,7 +80,6 @@ export class CompilePipe { } } - function _findPipeMeta(view: CompileView, name: string): CompilePipeMetadata { var pipeMeta: CompilePipeMetadata = null; for (var i = view.pipeMetas.length - 1; i >= 0; i--) { diff --git a/modules/@angular/compiler/src/view_compiler/compile_view.ts b/modules/@angular/compiler/src/view_compiler/compile_view.ts index c6197aaac8..c5d96a1533 100644 --- a/modules/@angular/compiler/src/view_compiler/compile_view.ts +++ b/modules/@angular/compiler/src/view_compiler/compile_view.ts @@ -127,16 +127,7 @@ export class CompileView implements NameResolver { } callPipe(name: string, input: o.Expression, args: o.Expression[]): o.Expression { - var compView = this.componentView; - var pipe = compView.purePipes.get(name); - if (isBlank(pipe)) { - pipe = new CompilePipe(compView, name); - if (pipe.pure) { - compView.purePipes.set(name, pipe); - } - compView.pipes.push(pipe); - } - return pipe.call(this, [input].concat(args)); + return CompilePipe.call(this, name, [input].concat(args)); } getLocal(name: string): o.Expression { diff --git a/modules/@angular/core/test/linker/change_detection_integration_spec.ts b/modules/@angular/core/test/linker/change_detection_integration_spec.ts index 2bf2aac892..fd4c7aef01 100644 --- a/modules/@angular/core/test/linker/change_detection_integration_spec.ts +++ b/modules/@angular/core/test/linker/change_detection_integration_spec.ts @@ -68,6 +68,7 @@ import { AfterViewInit, AfterViewChecked } from '@angular/core'; +import {NgFor} from '@angular/common'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {AsyncPipe} from '@angular/common'; @@ -544,16 +545,23 @@ export function main() { it('should call pure pipes that are used multiple times only when the arguments change', fakeAsync(() => { - var ctx = createCompFixture(`
`, Person); + var ctx = createCompFixture( + `
` + + '
', + Person); ctx.componentInstance.name = 'a'; ctx.componentInstance.age = 10; + ctx.componentInstance.address = new Address('mtv'); ctx.detectChanges(false); - expect(renderLog.loggedValues).toEqual(['a state:0', '10 state:1']); + expect(renderLog.loggedValues) + .toEqual(['mtv state:0', 'mtv state:1', 'a state:2', '10 state:3']); ctx.detectChanges(false); - expect(renderLog.loggedValues).toEqual(['a state:0', '10 state:1']); + expect(renderLog.loggedValues) + .toEqual(['mtv state:0', 'mtv state:1', 'a state:2', '10 state:3']); ctx.componentInstance.age = 11; ctx.detectChanges(false); - expect(renderLog.loggedValues).toEqual(['a state:0', '10 state:1', '11 state:2']); + expect(renderLog.loggedValues) + .toEqual(['mtv state:0', 'mtv state:1', 'a state:2', '10 state:3', '11 state:4']); })); it('should call impure pipes on each change detection run', fakeAsync(() => { @@ -1098,6 +1106,7 @@ const ALL_DIRECTIVES = /*@ts2dart_const*/[ forwardRef(() => OrderCheckDirective2), forwardRef(() => OrderCheckDirective0), forwardRef(() => OrderCheckDirective1), + NgFor ]; const ALL_PIPES = /*@ts2dart_const*/[ diff --git a/modules/@angular/core/test/linker/view_injector_integration_spec.ts b/modules/@angular/core/test/linker/view_injector_integration_spec.ts index 6b0e280f72..7b6db5363a 100644 --- a/modules/@angular/core/test/linker/view_injector_integration_spec.ts +++ b/modules/@angular/core/test/linker/view_injector_integration_spec.ts @@ -37,7 +37,7 @@ import { Host, SkipSelfMetadata } from '@angular/core'; -import {NgIf} from '@angular/common'; +import {NgIf, NgFor} from '@angular/common'; import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; const ALL_DIRECTIVES = /*@ts2dart_const*/[ @@ -71,7 +71,8 @@ const ALL_DIRECTIVES = /*@ts2dart_const*/[ forwardRef(() => DirectiveNeedsChangeDetectorRef), forwardRef(() => PushComponentNeedsChangeDetectorRef), forwardRef(() => NeedsHostAppService), - NgIf + NgIf, + NgFor ]; const ALL_PIPES = /*@ts2dart_const*/[ @@ -670,23 +671,35 @@ export function main() { it('should cache pure pipes', fakeAsync(() => { var el = createComp( - '
', + '
' + + '
', tcb); var purePipe1 = el.children[0].inject(SimpleDirective).value; var purePipe2 = el.children[1].inject(SimpleDirective).value; + var purePipe3 = el.children[2].inject(SimpleDirective).value; + var purePipe4 = el.children[3].inject(SimpleDirective).value; expect(purePipe1).toBeAnInstanceOf(PurePipe); - expect(purePipe1).toBe(purePipe2); + expect(purePipe2).toBe(purePipe1); + expect(purePipe3).toBe(purePipe1); + expect(purePipe4).toBe(purePipe1); })); - it('should not cache pure pipes', fakeAsync(() => { + it('should not cache impure pipes', fakeAsync(() => { var el = createComp( - '
', + '
' + + '
', tcb); var purePipe1 = el.children[0].inject(SimpleDirective).value; var purePipe2 = el.children[1].inject(SimpleDirective).value; + var purePipe3 = el.children[2].inject(SimpleDirective).value; + var purePipe4 = el.children[3].inject(SimpleDirective).value; expect(purePipe1).toBeAnInstanceOf(ImpurePipe); expect(purePipe2).toBeAnInstanceOf(ImpurePipe); - expect(purePipe1).not.toBe(purePipe2); + expect(purePipe2).not.toBe(purePipe1); + expect(purePipe3).toBeAnInstanceOf(ImpurePipe); + expect(purePipe3).not.toBe(purePipe1); + expect(purePipe4).toBeAnInstanceOf(ImpurePipe); + expect(purePipe4).not.toBe(purePipe1); })); }); });