From 19cfaf7f4cf3bdbc6676812ae804c4d897f1dce0 Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Tue, 3 Mar 2020 14:26:04 +0100 Subject: [PATCH] fix(core): don't re-invoke pure pipes that throw and arguments are the same (#35827) Pure pipes are not invoked again until their arguments are modified. The same rule should apply to pure pipes that throw an exception. This fix ensures that a pure pipe is not re-invoked if it throws an exception and arguments are not changed. PR Close #35827 --- packages/core/src/render3/pure_function.ts | 23 ++++- packages/core/test/acceptance/pipe_spec.ts | 107 +++++++++++++++++++++ 2 files changed, 125 insertions(+), 5 deletions(-) diff --git a/packages/core/src/render3/pure_function.ts b/packages/core/src/render3/pure_function.ts index b9d3e2b8d5..2594661166 100644 --- a/packages/core/src/render3/pure_function.ts +++ b/packages/core/src/render3/pure_function.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ +import {assertDataInRange} from '../util/assert'; import {bindingUpdated, bindingUpdated2, bindingUpdated3, bindingUpdated4, getBinding, updateBinding} from './bindings'; import {LView} from './interfaces/view'; import {getBindingRoot, getLView} from './state'; @@ -277,6 +278,18 @@ export function ɵɵpureFunctionV( return pureFunctionVInternal(getLView(), getBindingRoot(), slotOffset, pureFn, exps, thisArg); } +/** + * Results of a pure function invocation are stored in LView in a dedicated slot that is initialized + * to NO_CHANGE. In rare situations a pure pipe might throw an exception on the very first + * invocation and not produce any valid results. In this case LView would keep holding the NO_CHANGE + * value. The NO_CHANGE is not something that we can use in expressions / bindings thus we convert + * it to `undefined`. + */ +function getPureFunctionReturnValue(lView: LView, returnValueIndex: number) { + ngDevMode && assertDataInRange(lView, returnValueIndex); + const lastReturnValue = lView[returnValueIndex]; + return lastReturnValue === NO_CHANGE ? undefined : lastReturnValue; +} /** * If the value of the provided exp has changed, calls the pure function to return @@ -296,7 +309,7 @@ export function pureFunction1Internal( const bindingIndex = bindingRoot + slotOffset; return bindingUpdated(lView, bindingIndex, exp) ? updateBinding(lView, bindingIndex + 1, thisArg ? pureFn.call(thisArg, exp) : pureFn(exp)) : - getBinding(lView, bindingIndex + 1); + getPureFunctionReturnValue(lView, bindingIndex + 1); } @@ -321,7 +334,7 @@ export function pureFunction2Internal( updateBinding( lView, bindingIndex + 2, thisArg ? pureFn.call(thisArg, exp1, exp2) : pureFn(exp1, exp2)) : - getBinding(lView, bindingIndex + 2); + getPureFunctionReturnValue(lView, bindingIndex + 2); } /** @@ -347,7 +360,7 @@ export function pureFunction3Internal( updateBinding( lView, bindingIndex + 3, thisArg ? pureFn.call(thisArg, exp1, exp2, exp3) : pureFn(exp1, exp2, exp3)) : - getBinding(lView, bindingIndex + 3); + getPureFunctionReturnValue(lView, bindingIndex + 3); } @@ -376,7 +389,7 @@ export function pureFunction4Internal( updateBinding( lView, bindingIndex + 4, thisArg ? pureFn.call(thisArg, exp1, exp2, exp3, exp4) : pureFn(exp1, exp2, exp3, exp4)) : - getBinding(lView, bindingIndex + 4); + getPureFunctionReturnValue(lView, bindingIndex + 4); } /** @@ -403,5 +416,5 @@ export function pureFunctionVInternal( bindingUpdated(lView, bindingIndex++, exps[i]) && (different = true); } return different ? updateBinding(lView, bindingIndex, pureFn.apply(thisArg, exps)) : - getBinding(lView, bindingIndex); + getPureFunctionReturnValue(lView, bindingIndex); } diff --git a/packages/core/test/acceptance/pipe_spec.ts b/packages/core/test/acceptance/pipe_spec.ts index da9a561d2b..49c020337b 100644 --- a/packages/core/test/acceptance/pipe_spec.ts +++ b/packages/core/test/acceptance/pipe_spec.ts @@ -582,4 +582,111 @@ describe('pipe', () => { }); + describe('pure pipe error handling', () => { + + it('should not re-invoke pure pipes if it fails initially', () => { + + @Pipe({name: 'throwPipe', pure: true}) + class ThrowPipe implements PipeTransform { + transform(): never { throw new Error('ThrowPipeError'); } + } + @Component({template: `{{val | throwPipe}}`}) + class App { + val = 'anything'; + } + + const fixture = + TestBed.configureTestingModule({declarations: [App, ThrowPipe]}).createComponent(App); + + // first invocation + expect(() => fixture.detectChanges()).toThrowError(/ThrowPipeError/); + + // second invocation - should not throw + fixture.detectChanges(); + }); + + + it('should display the last known result from a pure pipe when it throws', () => { + + @Pipe({name: 'throwPipe', pure: true}) + class ThrowPipe implements PipeTransform { + transform(value: string): string { + if (value === 'KO') { + throw new Error('ThrowPipeError'); + } else { + return value; + } + } + } + + @Component({template: `{{val | throwPipe}}`}) + class App { + val = 'anything'; + } + + const fixture = + TestBed.configureTestingModule({declarations: [App, ThrowPipe]}).createComponent(App); + + // first invocation - no error thrown + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toBe('anything'); + + + // second invocation when the error is thrown + fixture.componentInstance.val = 'KO'; + expect(() => fixture.detectChanges()).toThrowError(/ThrowPipeError/); + expect(fixture.nativeElement.textContent).toBe('anything'); + + + // third invocation with no changes to input - should not thrown and preserve the last known + // results + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toBe('anything'); + }); + + describe('pure pipe error handling with multiple arguments', () => { + const args: string[] = new Array(10).fill(':0'); + for (let numberOfPipeArgs = 0; numberOfPipeArgs < args.length; numberOfPipeArgs++) { + it(`should not invoke ${numberOfPipeArgs} argument pure pipe second time if it throws unless input changes`, + () => { + // https://stackblitz.com/edit/angular-mbx2pg + const log: string[] = []; + @Pipe({name: 'throw', pure: true}) + class ThrowPipe implements PipeTransform { + transform(): never { + log.push('throw'); + throw new Error('ThrowPipeError'); + } + } + @Component({template: `{{val | throw${args.slice(0, numberOfPipeArgs).join('')}}}`}) + class App { + val = 'anything'; + } + + const fixture = TestBed.configureTestingModule({declarations: [App, ThrowPipe]}) + .createComponent(App); + // First invocation of detect changes should throw. + expect(() => fixture.detectChanges()).toThrowError(/ThrowPipeError/); + expect(log).toEqual(['throw']); + // Second invocation should not throw as input to the `throw` pipe has not changed and + // the pipe is pure. + log.length = 0; + expect(() => fixture.detectChanges()).not.toThrow(); + expect(log).toEqual([]); + fixture.componentInstance.val = 'change'; + // First invocation of detect changes should throw because the input changed. + expect(() => fixture.detectChanges()).toThrowError(/ThrowPipeError/); + expect(log).toEqual(['throw']); + // Second invocation should not throw as input to the `throw` pipe has not changed and + // the pipe is pure. + log.length = 0; + expect(() => fixture.detectChanges()).not.toThrow(); + expect(log).toEqual([]); + }); + } + }); + + }); + + });