From fc88a79b3217307b302407e915b52f390291ed45 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 30 Jan 2019 15:23:23 +0100 Subject: [PATCH] fix(ivy): errors not being logged to ErrorHandler (#28447) Fixes Ivy not passing thrown errors along to the `ErrorHandler`. **Note:** the failing test had to be reworked a little bit, because it has some assertions that depend on an error context being logged, however Ivy doesn't keep track of the error context. This PR resolves FW-840. PR Close #28447 --- packages/core/src/render3/instructions.ts | 40 +++-- .../bundling/todo/bundle.golden_symbols.json | 27 ++++ .../linker/regression_integration_spec.ts | 149 ++++++++++-------- 3 files changed, 137 insertions(+), 79 deletions(-) diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 84aae207a1..0ae70f4391 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -8,6 +8,7 @@ import {InjectFlags, InjectionToken, Injector} from '../di'; import {resolveForwardRef} from '../di/forward_ref'; +import {ErrorHandler} from '../error_handler'; import {Type} from '../interface/type'; import {validateAttribute, validateProperty} from '../sanitization/sanitization'; import {Sanitizer} from '../sanitization/security'; @@ -936,6 +937,7 @@ function listenerInternal( const tView = lView[TVIEW]; const firstTemplatePass = tView.firstTemplatePass; const tCleanup: false|any[] = firstTemplatePass && (tView.cleanup || (tView.cleanup = [])); + ngDevMode && assertNodeOfPossibleTypes( tNode, TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer); @@ -2605,13 +2607,17 @@ function wrapListener( markViewDirty(startView); } - const result = listenerFn(e); - if (wrapWithPreventDefault && result === false) { - e.preventDefault(); - // Necessary for legacy browsers that don't support preventDefault (e.g. IE) - e.returnValue = false; + try { + const result = listenerFn(e); + if (wrapWithPreventDefault && result === false) { + e.preventDefault(); + // Necessary for legacy browsers that don't support preventDefault (e.g. IE) + e.returnValue = false; + } + return result; + } catch (error) { + handleError(lView, error); } - return result; }; } /** @@ -2723,12 +2729,17 @@ export function detectChangesInternal(view: LView, context: T) { if (rendererFactory.begin) rendererFactory.begin(); - if (isCreationMode(view)) { - checkView(view, context); // creation mode pass + try { + if (isCreationMode(view)) { + checkView(view, context); // creation mode pass + } + checkView(view, context); // update mode pass + } catch (error) { + handleError(view, error); + throw error; + } finally { + if (rendererFactory.end) rendererFactory.end(); } - checkView(view, context); // update mode pass - - if (rendererFactory.end) rendererFactory.end(); } /** @@ -3237,3 +3248,10 @@ function loadComponentRenderer(tNode: TNode, lView: LView): Renderer3 { const componentLView = lView[tNode.index] as LView; return componentLView[RENDERER]; } + +/** Handles an error thrown in an LView. */ +function handleError(lView: LView, error: any): void { + const injector = lView[INJECTOR]; + const errorHandler = injector ? injector.get(ErrorHandler, null) : null; + errorHandler && errorHandler.handleError(error); +} diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 5a68a1ff42..b7aa53f597 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -50,12 +50,24 @@ { "name": "EMPTY_OBJ" }, + { + "name": "ERROR_DEBUG_CONTEXT" + }, + { + "name": "ERROR_LOGGER" + }, + { + "name": "ERROR_ORIGINAL_ERROR" + }, { "name": "ElementRef" }, { "name": "EmptyErrorImpl" }, + { + "name": "ErrorHandler" + }, { "name": "FLAGS" }, @@ -491,6 +503,9 @@ { "name": "decreaseElementDepthCount" }, + { + "name": "defaultErrorLogger" + }, { "name": "defaultScheduler" }, @@ -641,6 +656,9 @@ { "name": "getCurrentView" }, + { + "name": "getDebugContext" + }, { "name": "getDirectiveDef" }, @@ -656,6 +674,9 @@ { "name": "getElementDepthCount" }, + { + "name": "getErrorLogger" + }, { "name": "getHighestElementOrICUContainer" }, @@ -722,6 +743,9 @@ { "name": "getOrCreateTView" }, + { + "name": "getOriginalError" + }, { "name": "getParentInjectorIndex" }, @@ -803,6 +827,9 @@ { "name": "getValue" }, + { + "name": "handleError" + }, { "name": "hasClassInput" }, diff --git a/packages/core/test/linker/regression_integration_spec.ts b/packages/core/test/linker/regression_integration_spec.ts index e6c0dbd65f..8c19283f64 100644 --- a/packages/core/test/linker/regression_integration_spec.ts +++ b/packages/core/test/linker/regression_integration_spec.ts @@ -450,84 +450,97 @@ function declareTestsUsingBootstrap() { if (getDOM().supportsDOMEvents()) { // This test needs a real DOM.... - fixmeIvy('FW-840: Exceptions thrown in event handlers are not reported to ErrorHandler') - .it('should keep change detecting if there was an error', (done) => { - @Component({ - selector: COMP_SELECTOR, - template: - 'Value:{{value}}{{throwIfNeeded()}}' - }) - class ErrorComp { - value = 0; - thrownValue = 0; - next() { this.value++; } - nextAndThrow() { - this.value++; - this.throwIfNeeded(); - } - throwIfNeeded() { - NgZone.assertInAngularZone(); - if (this.thrownValue !== this.value) { - this.thrownValue = this.value; - throw new Error(`Error: ${this.value}`); - } - } + it('should keep change detecting if there was an error', (done) => { + @Component({ + selector: COMP_SELECTOR, + template: + 'Value:{{value}}{{throwIfNeeded()}}' + }) + class ErrorComp { + value = 0; + thrownValue = 0; + next() { this.value++; } + nextAndThrow() { + this.value++; + this.throwIfNeeded(); + } + throwIfNeeded() { + NgZone.assertInAngularZone(); + if (this.thrownValue !== this.value) { + this.thrownValue = this.value; + throw new Error(`Error: ${this.value}`); } + } + } - @Directive({selector: '[dirClick]'}) - class EventDir { - @Output() - dirClick = new EventEmitter(); + @Directive({selector: '[dirClick]'}) + class EventDir { + @Output() + dirClick = new EventEmitter(); - @HostListener('click', ['$event']) - onClick(event: any) { this.dirClick.next(event); } - } + @HostListener('click', ['$event']) + onClick(event: any) { this.dirClick.next(event); } + } - @NgModule({ - imports: [BrowserModule], - declarations: [ErrorComp, EventDir], - bootstrap: [ErrorComp], - providers: [{provide: ErrorHandler, useValue: errorHandler}], - }) - class TestModule { - } + @NgModule({ + imports: [BrowserModule], + declarations: [ErrorComp, EventDir], + bootstrap: [ErrorComp], + providers: [{provide: ErrorHandler, useValue: errorHandler}], + }) + class TestModule { + } - platformBrowserDynamic().bootstrapModule(TestModule).then((ref) => { - NgZone.assertNotInAngularZone(); - const appRef = ref.injector.get(ApplicationRef) as ApplicationRef; - const compRef = appRef.components[0] as ComponentRef; - const compEl = compRef.location.nativeElement; - const nextBtn = compEl.children[0]; - const nextAndThrowBtn = compEl.children[1]; - const nextAndThrowDirBtn = compEl.children[2]; + platformBrowserDynamic().bootstrapModule(TestModule).then((ref) => { + NgZone.assertNotInAngularZone(); + const appRef = ref.injector.get(ApplicationRef) as ApplicationRef; + const compRef = appRef.components[0] as ComponentRef; + const compEl = compRef.location.nativeElement; + const nextBtn = compEl.children[0]; + const nextAndThrowBtn = compEl.children[1]; + const nextAndThrowDirBtn = compEl.children[2]; - nextBtn.click(); - assertValueAndErrors(compEl, 1, 0); - nextBtn.click(); - assertValueAndErrors(compEl, 2, 2); + // Note: the amount of events sent to the logger will differ between ViewEngine + // and Ivy, because Ivy doesn't attach an error context. This means that the amount + // of logged errors increases by 1 for Ivy and 2 for ViewEngine after each event. + const errorDelta = ivyEnabled ? 1 : 2; + let currentErrorIndex = 0; - nextAndThrowBtn.click(); - assertValueAndErrors(compEl, 3, 4); - nextAndThrowBtn.click(); - assertValueAndErrors(compEl, 4, 6); + nextBtn.click(); + assertValueAndErrors(compEl, 1, currentErrorIndex); + currentErrorIndex += errorDelta; + nextBtn.click(); + assertValueAndErrors(compEl, 2, currentErrorIndex); + currentErrorIndex += errorDelta; - nextAndThrowDirBtn.click(); - assertValueAndErrors(compEl, 5, 8); - nextAndThrowDirBtn.click(); - assertValueAndErrors(compEl, 6, 10); + nextAndThrowBtn.click(); + assertValueAndErrors(compEl, 3, currentErrorIndex); + currentErrorIndex += errorDelta; + nextAndThrowBtn.click(); + assertValueAndErrors(compEl, 4, currentErrorIndex); + currentErrorIndex += errorDelta; - // Assert that there were no more errors - expect(logger.errors.length).toBe(12); - done(); - }); + nextAndThrowDirBtn.click(); + assertValueAndErrors(compEl, 5, currentErrorIndex); + currentErrorIndex += errorDelta; + nextAndThrowDirBtn.click(); + assertValueAndErrors(compEl, 6, currentErrorIndex); + currentErrorIndex += errorDelta; - function assertValueAndErrors(compEl: any, value: number, errorIndex: number) { - expect(compEl).toHaveText(`Value:${value}`); - expect(logger.errors[errorIndex][0]).toBe('ERROR'); - expect(logger.errors[errorIndex][1].message).toBe(`Error: ${value}`); - expect(logger.errors[errorIndex + 1][0]).toBe('ERROR CONTEXT'); - } - }); + // Assert that there were no more errors + expect(logger.errors.length).toBe(currentErrorIndex); + done(); + }); + + function assertValueAndErrors(compEl: any, value: number, errorIndex: number) { + expect(compEl).toHaveText(`Value:${value}`); + expect(logger.errors[errorIndex][0]).toBe('ERROR'); + expect(logger.errors[errorIndex][1].message).toBe(`Error: ${value}`); + + // Ivy doesn't attach an error context. + !ivyEnabled && expect(logger.errors[errorIndex + 1][0]).toBe('ERROR CONTEXT'); + } + }); } }); }