Skip to content

Commit

Permalink
fix(core): ErrorHandler should not rethrow an error by default (angul…
Browse files Browse the repository at this point in the history
…ar#15077) (angular#15208)

ErrorHandler can not throw errors because it will unsubscribe itself from
the error stream.

Zones captures errors and feed it into NgZone, which than has a Rx Observable
to feed it into ErrorHandler. If the ErroHandler throws, then Rx will teardown
the observable which in essence causes the ErrorHandler to be removed from the
error handling. This implies that the ErrorHandler can never throw errors.

Closes angular#14949
Closes angular#15182
Closes angular#14316
  • Loading branch information
mhevery authored and Zhicheng Wang committed Aug 11, 2017
1 parent 179cff6 commit 5672ea4
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 18 deletions.
16 changes: 6 additions & 10 deletions packages/core/src/error_handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ export class ErrorHandler {
*/
_console: Console = console;

/**
* @internal
*/
rethrowError: boolean;

constructor(rethrowError: boolean = true) { this.rethrowError = rethrowError; }
constructor(
/**
* @deprecated since v4.0 parameter no longer has an effect, as ErrorHandler will never
* rethrow.
*/
deprecatedParameter?: boolean) {}

handleError(error: any): void {
const originalError = this._findOriginalError(error);
Expand All @@ -63,10 +63,6 @@ export class ErrorHandler {
if (context) {
errorLogger(this._console, 'ERROR CONTEXT', context);
}

// We rethrow exceptions, so operations like 'bootstrap' will result in an error
// when an error happens. If we do not rethrow, bootstrap will always succeed.
if (this.rethrowError) throw error;
}

/** @internal */
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/application_ref_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function main() {
} else {
options = providersOrOptions || {};
}
const errorHandler = new ErrorHandler(false);
const errorHandler = new ErrorHandler();
errorHandler._console = mockConsole as any;

const platformModule = getDOM().supportsDOMEvents() ? BrowserModule : ServerModule;
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/error_handler_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class MockConsole {
export function main() {
function errorToString(error: any) {
const logger = new MockConsole();
const errorHandler = new ErrorHandler(false);
const errorHandler = new ErrorHandler();
errorHandler._console = logger as any;
errorHandler.handleError(error);
return logger.res.map(line => line.join('#')).join('\n');
Expand Down Expand Up @@ -61,7 +61,7 @@ ERROR CONTEXT#Context`);
it('should use the error logger on the error', () => {
const err = new Error('test');
const console = new MockConsole();
const errorHandler = new ErrorHandler(false);
const errorHandler = new ErrorHandler();
errorHandler._console = console as any;
const logger = jasmine.createSpy('logger');
(err as any)[ERROR_LOGGER] = logger;
Expand Down
8 changes: 4 additions & 4 deletions packages/platform-browser/test/browser/bootstrap_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ export function main() {
it('should throw if bootstrapped Directive is not a Component',
inject([AsyncTestCompleter], (done: AsyncTestCompleter) => {
const logger = new MockConsole();
const errorHandler = new ErrorHandler(false);
const errorHandler = new ErrorHandler();
errorHandler._console = logger as any;
expect(
() => bootstrap(
Expand All @@ -171,7 +171,7 @@ export function main() {
it('should throw if no element is found',
inject([AsyncTestCompleter], (async: AsyncTestCompleter) => {
const logger = new MockConsole();
const errorHandler = new ErrorHandler(false);
const errorHandler = new ErrorHandler();
errorHandler._console = logger as any;
bootstrap(NonExistentComp, [
{provide: ErrorHandler, useValue: errorHandler}
Expand All @@ -187,7 +187,7 @@ export function main() {
it('should forward the error to promise when bootstrap fails',
inject([AsyncTestCompleter], (async: AsyncTestCompleter) => {
const logger = new MockConsole();
const errorHandler = new ErrorHandler(false);
const errorHandler = new ErrorHandler();
errorHandler._console = logger as any;

const refPromise =
Expand All @@ -202,7 +202,7 @@ export function main() {
it('should invoke the default exception handler when bootstrap fails',
inject([AsyncTestCompleter], (async: AsyncTestCompleter) => {
const logger = new MockConsole();
const errorHandler = new ErrorHandler(false);
const errorHandler = new ErrorHandler();
errorHandler._console = logger as any;

const refPromise =
Expand Down
3 changes: 2 additions & 1 deletion tools/public_api_guard/core/typings/core.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,8 @@ export declare function enableProdMode(): void;

/** @stable */
export declare class ErrorHandler {
constructor(rethrowError?: boolean);
constructor(
deprecatedParameter?: boolean);
handleError(error: any): void;
}

Expand Down

0 comments on commit 5672ea4

Please sign in to comment.