From 8543c347a8cd2fa7a96dcac7ea1da572f86586fa Mon Sep 17 00:00:00 2001 From: vsavkin Date: Mon, 27 Jul 2015 15:47:42 -0700 Subject: [PATCH] feat(core): provide an error context when an exception happens in an error handler --- .../abstract_change_detector.ts | 5 +- .../angular2/src/core/application_common.ts | 41 +++++---- .../src/core/compiler/element_injector.ts | 2 +- modules/angular2/src/core/compiler/view.ts | 90 ++++++++++++------- .../angular2/src/core/exception_handler.ts | 2 +- modules/angular2/src/facade/lang.dart | 2 + modules/angular2/src/facade/lang.ts | 2 + modules/angular2/src/test_lib/fake_async.dart | 7 ++ modules/angular2/src/test_lib/fake_async.ts | 12 ++- modules/angular2/src/test_lib/test_lib.ts | 1 + .../change_detection/change_detector_spec.ts | 4 +- .../angular2/test/core/application_spec.ts | 34 +++++-- .../test/core/compiler/integration_spec.ts | 40 ++++++++- 13 files changed, 175 insertions(+), 67 deletions(-) diff --git a/modules/angular2/src/change_detection/abstract_change_detector.ts b/modules/angular2/src/change_detection/abstract_change_detector.ts index 7271ae8978..8f584b9d08 100644 --- a/modules/angular2/src/change_detection/abstract_change_detector.ts +++ b/modules/angular2/src/change_detection/abstract_change_detector.ts @@ -90,8 +90,9 @@ export class AbstractChangeDetector implements ChangeDetector { throwError(proto: ProtoRecord, exception: any, stack: any): void { var c = this.dispatcher.getDebugContext(proto.bindingRecord.elementIndex, proto.directiveIndex); - var context = new _Context(c["element"], c["componentElement"], c["directive"], c["context"], - c["locals"], c["injector"], proto.expressionAsString); + var context = isPresent(c) ? new _Context(c.element, c.componentElement, c.directive, c.context, + c.locals, c.injector, proto.expressionAsString) : + null; throw new ChangeDetectionError(proto, exception, stack, context); } } \ No newline at end of file diff --git a/modules/angular2/src/core/application_common.ts b/modules/angular2/src/core/application_common.ts index 97817e1a44..501b1094de 100644 --- a/modules/angular2/src/core/application_common.ts +++ b/modules/angular2/src/core/application_common.ts @@ -7,7 +7,8 @@ import { BaseException, assertionsEnabled, print, - stringify + stringify, + isDart } from 'angular2/src/facade/lang'; import {BrowserDomAdapter} from 'angular2/src/dom/browser_adapter'; import {DOM} from 'angular2/src/dom/dom_adapter'; @@ -128,7 +129,7 @@ function _injectorBindings(appComponentType): List> { DirectiveResolver, Parser, Lexer, - bind(ExceptionHandler).toFactory(() => new ExceptionHandler(DOM), []), + bind(ExceptionHandler).toFactory(() => new ExceptionHandler(DOM, isDart ? false : true), []), bind(XHR).toValue(new XHRImpl()), ComponentUrlMapper, UrlResolver, @@ -280,8 +281,8 @@ export function commonBootstrap( appComponentType: /*Type*/ any, componentInjectableBindings: List> = null): Promise { BrowserDomAdapter.makeCurrent(); - var bootstrapProcess: PromiseCompleter = PromiseWrapper.completer(); - var zone = createNgZone(new ExceptionHandler(DOM)); + var bootstrapProcess = PromiseWrapper.completer(); + var zone = createNgZone(new ExceptionHandler(DOM, isDart ? false : true)); zone.run(() => { // TODO(rado): prepopulate template cache, so applications with only // index.html and main.js are possible. @@ -290,19 +291,27 @@ export function commonBootstrap( var exceptionHandler = appInjector.get(ExceptionHandler); zone.overrideOnErrorHandler((e, s) => exceptionHandler.call(e, s)); - var compRefToken: Promise = - PromiseWrapper.wrap(() => appInjector.get(appComponentRefPromiseToken)); - var tick = (componentRef) => { - var appChangeDetector = internalView(componentRef.hostView).changeDetector; - // retrieve life cycle: may have already been created if injected in root component - var lc = appInjector.get(LifeCycle); - lc.registerWith(zone, appChangeDetector); - lc.tick(); // the first tick that will bootstrap the app + try { + var compRefToken: Promise = appInjector.get(appComponentRefPromiseToken); + var tick = (componentRef) => { + var appChangeDetector = internalView(componentRef.hostView).changeDetector; + // retrieve life cycle: may have already been created if injected in root component + var lc = appInjector.get(LifeCycle); + lc.registerWith(zone, appChangeDetector); + lc.tick(); // the first tick that will bootstrap the app - bootstrapProcess.resolve(new ApplicationRef(componentRef, appComponentType, appInjector)); - }; - PromiseWrapper.then(compRefToken, tick, - (err, stackTrace) => bootstrapProcess.reject(err, stackTrace)); + bootstrapProcess.resolve(new ApplicationRef(componentRef, appComponentType, appInjector)); + }; + + var tickResult = PromiseWrapper.then(compRefToken, tick); + + PromiseWrapper.then(tickResult, + (_) => {}); // required for Dart to trigger the default error handler + PromiseWrapper.then(tickResult, null, + (err, stackTrace) => { bootstrapProcess.reject(err, stackTrace); }); + } catch (e) { + bootstrapProcess.reject(e, e.stack); + } }); return bootstrapProcess.promise; diff --git a/modules/angular2/src/core/compiler/element_injector.ts b/modules/angular2/src/core/compiler/element_injector.ts index ad9aea6c95..31e66f4368 100644 --- a/modules/angular2/src/core/compiler/element_injector.ts +++ b/modules/angular2/src/core/compiler/element_injector.ts @@ -502,7 +502,7 @@ export class ElementInjector extends TreeNode implements Depend var p = this._preBuiltObjects; var index = p.elementRef.boundElementIndex - p.view.elementOffset; var c = this._preBuiltObjects.view.getDebugContext(index, null); - return new _Context(c["element"], c["componentElement"], c["injector"]); + return isPresent(c) ? new _Context(c.element, c.componentElement, c.injector) : null; } private _reattachInjectors(imperativelyCreatedInjector: Injector): void { diff --git a/modules/angular2/src/core/compiler/view.ts b/modules/angular2/src/core/compiler/view.ts index 043f42d2cf..a4ed079186 100644 --- a/modules/angular2/src/core/compiler/view.ts +++ b/modules/angular2/src/core/compiler/view.ts @@ -212,7 +212,7 @@ export class AppView implements ChangeDispatcher, RenderEventDispatcher { return isPresent(boundElementIndex) ? this.elementRefs[boundElementIndex] : null; } - getDebugContext(elementIndex: number, directiveIndex: DirectiveIndex): StringMap { + getDebugContext(elementIndex: number, directiveIndex: DirectiveIndex): DebugContext { try { var offsettedIndex = this.elementOffset + elementIndex; var hasRefForIndex = offsettedIndex < this.elementRefs.length; @@ -226,18 +226,13 @@ export class AppView implements ChangeDispatcher, RenderEventDispatcher { var directive = isPresent(directiveIndex) ? this.getDirectiveFor(directiveIndex) : null; var injector = isPresent(ei) ? ei.getInjector() : null; - return { - element: element, - componentElement: componentElement, - directive: directive, - context: this.context, - locals: _localsToStringMap(this.locals), - injector: injector - }; + return new DebugContext(element, componentElement, directive, this.context, + _localsToStringMap(this.locals), injector); + } catch (e) { // TODO: vsavkin log the exception once we have a good way to log errors and warnings // if an error happens during getting the debug context, we return an empty map. - return {}; + return null; } } @@ -262,29 +257,37 @@ export class AppView implements ChangeDispatcher, RenderEventDispatcher { // returns false if preventDefault must be applied to the DOM event dispatchEvent(boundElementIndex: number, eventName: string, locals: Map): boolean { - // Most of the time the event will be fired only when the view is in the live document. - // However, in a rare circumstance the view might get dehydrated, in between the event - // queuing up and firing. - var allowDefaultBehavior = true; - if (this.hydrated()) { - var elBinder = this.proto.elementBinders[boundElementIndex - this.elementOffset]; - if (isBlank(elBinder.hostListeners)) return allowDefaultBehavior; - var eventMap = elBinder.hostListeners[eventName]; - if (isBlank(eventMap)) return allowDefaultBehavior; - MapWrapper.forEach(eventMap, (expr, directiveIndex) => { - var context; - if (directiveIndex === -1) { - context = this.context; - } else { - context = this.elementInjectors[boundElementIndex].getDirectiveAtIndex(directiveIndex); - } - var result = expr.eval(context, new Locals(this.locals, locals)); - if (isPresent(result)) { - allowDefaultBehavior = allowDefaultBehavior && result == true; - } - }); + try { + // Most of the time the event will be fired only when the view is in the live document. + // However, in a rare circumstance the view might get dehydrated, in between the event + // queuing up and firing. + var allowDefaultBehavior = true; + if (this.hydrated()) { + var elBinder = this.proto.elementBinders[boundElementIndex - this.elementOffset]; + if (isBlank(elBinder.hostListeners)) return allowDefaultBehavior; + var eventMap = elBinder.hostListeners[eventName]; + if (isBlank(eventMap)) return allowDefaultBehavior; + MapWrapper.forEach(eventMap, (expr, directiveIndex) => { + var context; + if (directiveIndex === -1) { + context = this.context; + } else { + context = this.elementInjectors[boundElementIndex].getDirectiveAtIndex(directiveIndex); + } + var result = expr.eval(context, new Locals(this.locals, locals)); + if (isPresent(result)) { + allowDefaultBehavior = allowDefaultBehavior && result == true; + } + }); + } + return allowDefaultBehavior; + } catch (e) { + var c = this.getDebugContext(boundElementIndex - this.elementOffset, null); + var context = isPresent(c) ? new _Context(c.element, c.componentElement, c.context, c.locals, + c.injector) : + null; + throw new EventEvaluationError(eventName, e, e.stack, context); } - return allowDefaultBehavior; } } @@ -298,6 +301,29 @@ function _localsToStringMap(locals: Locals): StringMap { return res; } +export class DebugContext { + constructor(public element: any, public componentElement: any, public directive: any, + public context: any, public locals: any, public injector: any) {} +} + +/** + * Error context included when an event handler throws an exception. + */ +class _Context { + constructor(public element: any, public componentElement: any, public context: any, + public locals: any, public injector: any) {} +} + +/** + * Wraps an exception thrown by an event handler. + */ +class EventEvaluationError extends BaseException { + constructor(eventName: string, originalException: any, originalStack: any, context: any) { + super(`Error during evaluation of "${eventName}"`, originalException, originalStack, context); + } +} + + /** * */ diff --git a/modules/angular2/src/core/exception_handler.ts b/modules/angular2/src/core/exception_handler.ts index b329a004c1..749622f842 100644 --- a/modules/angular2/src/core/exception_handler.ts +++ b/modules/angular2/src/core/exception_handler.ts @@ -80,7 +80,7 @@ export class ExceptionHandler { _longStackTrace(stackTrace: any): any { return isListLikeIterable(stackTrace) ? (stackTrace).join("\n\n-----async gap-----\n") : - stackTrace; + stackTrace.toString(); } _findContext(exception: any): any { diff --git a/modules/angular2/src/facade/lang.dart b/modules/angular2/src/facade/lang.dart index 57bd90b6e7..d7d5570c5c 100644 --- a/modules/angular2/src/facade/lang.dart +++ b/modules/angular2/src/facade/lang.dart @@ -5,6 +5,8 @@ import 'dart:math' as math; import 'dart:convert' as convert; import 'dart:async' show Future; +bool isDart = true; + String getTypeNameForDebugging(Type type) => type.toString(); class Math { diff --git a/modules/angular2/src/facade/lang.ts b/modules/angular2/src/facade/lang.ts index a061daef04..94da79057f 100644 --- a/modules/angular2/src/facade/lang.ts +++ b/modules/angular2/src/facade/lang.ts @@ -9,6 +9,8 @@ export function getTypeNameForDebugging(type: Type): string { return type['name']; } +export var isDart = false; + export class BaseException extends Error { stack; constructor(public message?: string, private _originalException?, private _originalStack?, diff --git a/modules/angular2/src/test_lib/fake_async.dart b/modules/angular2/src/test_lib/fake_async.dart index c7b05e0650..3e3d9ca8ce 100644 --- a/modules/angular2/src/test_lib/fake_async.dart +++ b/modules/angular2/src/test_lib/fake_async.dart @@ -80,6 +80,13 @@ void tick([int millis = 0]) { _fakeAsync.elapse(duration); } +/** + * This is not needed in Dart. Because quiver correctly removes a timer when + * it throws an exception. + */ +void clearPendingTimers() { +} + /** * Flush any pending microtasks. */ diff --git a/modules/angular2/src/test_lib/fake_async.ts b/modules/angular2/src/test_lib/fake_async.ts index f6c157601e..4c9a9870ed 100644 --- a/modules/angular2/src/test_lib/fake_async.ts +++ b/modules/angular2/src/test_lib/fake_async.ts @@ -41,9 +41,7 @@ export function fakeAsync(fn: Function): Function { return function(...args) { // TODO(tbosch): This class should already be part of the jasmine typings but it is not... _scheduler = new (jasmine).DelayedFunctionScheduler(); - ListWrapper.clear(_microtasks); - ListWrapper.clear(_pendingPeriodicTimers); - ListWrapper.clear(_pendingTimers); + clearPendingTimers(); let res = fakeAsyncZone.run(() => { let res = fn(...args); @@ -67,6 +65,14 @@ export function fakeAsync(fn: Function): Function { } } +// TODO we should fix tick to dequeue the failed timer instead of relying on clearPendingTimers +export function clearPendingTimers() { + ListWrapper.clear(_microtasks); + ListWrapper.clear(_pendingPeriodicTimers); + ListWrapper.clear(_pendingTimers); +} + + /** * Simulates the asynchronous passage of time for the timers in the fakeAsync zone. * diff --git a/modules/angular2/src/test_lib/test_lib.ts b/modules/angular2/src/test_lib/test_lib.ts index 3233964299..02e13ae546 100644 --- a/modules/angular2/src/test_lib/test_lib.ts +++ b/modules/angular2/src/test_lib/test_lib.ts @@ -32,6 +32,7 @@ export interface NgMatchers extends jasmine.Matchers { export var expect: (actual: any) => NgMatchers = _global.expect; +// TODO vsavkin: remove it and use lang/isDart instead export var IS_DARTIUM = false; export class AsyncTestCompleter { diff --git a/modules/angular2/test/change_detection/change_detector_spec.ts b/modules/angular2/test/change_detection/change_detector_spec.ts index f74e2ed694..72c54c5758 100644 --- a/modules/angular2/test/change_detection/change_detector_spec.ts +++ b/modules/angular2/test/change_detection/change_detector_spec.ts @@ -1043,9 +1043,7 @@ class TestDispatcher implements ChangeDispatcher { notifyOnAllChangesDone() { this.onAllChangesDoneCalled = true; } - getDebugContext(a, b) { - return {} - } + getDebugContext(a, b) { return null; } _asString(value) { return (isBlank(value) ? 'null' : value.toString()); } } diff --git a/modules/angular2/test/core/application_spec.ts b/modules/angular2/test/core/application_spec.ts index f1b80b0496..56d982c8ca 100644 --- a/modules/angular2/test/core/application_spec.ts +++ b/modules/angular2/test/core/application_spec.ts @@ -66,12 +66,14 @@ class HelloRootMissingTemplate { class HelloRootDirectiveIsNotCmp { } -class _NilLogger { - log(s: any): void {} - logGroup(s: any): void {} +class _ArrayLogger { + res: any[] = []; + log(s: any): void { this.res.push(s); } + logGroup(s: any): void { this.res.push(s); } logGroupEnd(){}; } + export function main() { var fakeDoc, el, el2, testBindings, lightDom; @@ -90,7 +92,7 @@ export function main() { it('should throw if bootstrapped Directive is not a Component', inject([AsyncTestCompleter], (async) => { - var refPromise = bootstrap(HelloRootDirectiveIsNotCmp, testBindings); + var refPromise = bootstrap(HelloRootDirectiveIsNotCmp, [testBindings]); PromiseWrapper.then(refPromise, null, (exception) => { expect(exception).toContainError( @@ -101,10 +103,11 @@ export function main() { })); it('should throw if no element is found', inject([AsyncTestCompleter], (async) => { - // do not print errors to the console - var e = new ExceptionHandler(new _NilLogger()); + var logger = new _ArrayLogger(); + var exceptionHandler = new ExceptionHandler(logger, IS_DARTIUM ? false : true); - var refPromise = bootstrap(HelloRootCmp, [bind(ExceptionHandler).toValue(e)]); + var refPromise = + bootstrap(HelloRootCmp, [bind(ExceptionHandler).toValue(exceptionHandler)]); PromiseWrapper.then(refPromise, null, (reason) => { expect(reason.message).toContain('The selector "hello-app" did not match any elements'); async.done(); @@ -112,6 +115,23 @@ export function main() { }); })); + if (DOM.supportsDOMEvents()) { + it('should invoke the default exception handler when bootstrap fails', + inject([AsyncTestCompleter], (async) => { + var logger = new _ArrayLogger(); + var exceptionHandler = new ExceptionHandler(logger, IS_DARTIUM ? false : true); + + var refPromise = + bootstrap(HelloRootCmp, [bind(ExceptionHandler).toValue(exceptionHandler)]); + PromiseWrapper.then(refPromise, null, (reason) => { + expect(logger.res.join("")) + .toContain('The selector "hello-app" did not match any elements'); + async.done(); + return null; + }); + })); + } + it('should create an injector promise', () => { var refPromise = bootstrap(HelloRootCmp, testBindings); expect(refPromise).not.toBe(null); diff --git a/modules/angular2/test/core/compiler/integration_spec.ts b/modules/angular2/test/core/compiler/integration_spec.ts index ef15f83779..6c5d076204 100644 --- a/modules/angular2/test/core/compiler/integration_spec.ts +++ b/modules/angular2/test/core/compiler/integration_spec.ts @@ -16,7 +16,9 @@ import { containsRegexp, stringifyElement, TestComponentBuilder, - fakeAsync + fakeAsync, + tick, + clearPendingTimers } from 'angular2/test_lib'; @@ -1200,7 +1202,7 @@ export function main() { expect(c.injector).toBeAnInstanceOf(Injector); expect(c.expression).toContain("one.two.three"); expect(c.context).toBe(rootTC.componentInstance); - expect(c.locals["local"]).not.toBeNull(); + expect(c.locals["local"]).toBeDefined(); } async.done(); @@ -1226,6 +1228,38 @@ export function main() { }); })); + if (DOM.supportsDOMEvents()) { // this is required to use fakeAsync + it('should provide an error context when an error happens in an event handler', + inject([TestComponentBuilder], fakeAsync((tcb: TestComponentBuilder) => { + + tcb = tcb.overrideView(MyComp, new viewAnn.View({ + template: ``, + directives: [DirectiveEmitingEvent, DirectiveListeningEvent] + })); + + var rootTC; + tcb.createAsync(MyComp).then(root => { rootTC = root; }); + tick(); + + var tc = rootTC.componentViewChildren[0]; + tc.inject(DirectiveEmitingEvent).fireEvent("boom"); + + try { + tick(); + throw "Should throw"; + } catch (e) { + clearPendingTimers(); + + var c = e.context; + expect(DOM.nodeName(c.element).toUpperCase()).toEqual("SPAN"); + expect(DOM.nodeName(c.componentElement).toUpperCase()).toEqual("DIV"); + expect(c.injector).toBeAnInstanceOf(Injector); + expect(c.context).toBe(rootTC.componentInstance); + expect(c.locals["local"]).toBeDefined(); + } + }))); + } + if (!IS_DARTIUM) { it('should report a meaningful error when a directive is undefined', inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, @@ -1513,6 +1547,8 @@ class MyComp { this.ctxNumProp = 0; this.ctxBoolProp = false; } + + throwError() { throw 'boom'; } } @Component({selector: 'child-cmp', properties: ['dirProp'], viewInjector: [MyService]})