Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add global error/exception handler #174

Closed
sylvain-hamel opened this issue Jul 30, 2015 · 38 comments
Closed

Add global error/exception handler #174

sylvain-hamel opened this issue Jul 30, 2015 · 38 comments

Comments

@sylvain-hamel
Copy link

Aurelia should route any uncaught exception or any unhandled rejected promise coming from expressions or from methods called by the framework itself (attached, activate, constructor (DI)) to a global exception handling service in order to let application developers to log the errors.

This would be equivalent to Angular’s $exceptionHandler service.

This serves two purposes:

  1. At dev time, I can implement a handler that shows an alert whenever there is an uncaught exception. No need to constantly monitor the console window.
  2. At production time, I can implement a handler that forwards all uncaught exceptions to a logging service on my server. Moreover I could decide to show an error panel of some sort, something like “Oups! Something went wrong, please reload your browser.”
@jdanyow
Copy link
Contributor

jdanyow commented Jul 30, 2015

related to aurelia/router#2

@EisenbergEffect
Copy link
Contributor

Errors are going through the logger. To route them elsewhere, you can attach a custom log appender.

@sylvain-hamel
Copy link
Author

@EisenbergEffect I think using a custom log appender is fine.

However, it does not report all errors (see examples below). I think that all Aurelia components have to make sure they send errors to the logger if the logger is the way to handle errors.

What do you think?


Example 1

I have a view that does that:

  activate(params) {
    this.subscriptions.push(this.observerLocator.getObserver(this.router, 'currentInstruction').subscribe((currentInstruction)=>{
       this.invalid_memeber.invalid_function();
    }));
  }

and when invalid_function fails, the custom logger is not called. I see in the console:

Uncaught TypeError: Cannot read property 'invalid_function' of undefined
(anonymous function)@my-view.js:39
trigger@aurelia-binding.js:3516
handleChanges@aurelia-binding.js:3460

A workaround for this one is to handle it via window.addEventListener('error', (errorEvent)=>{...});

Example 2

I have a view that does this:

this.someService.someMethod().then((result) => {
  this.result = result
});

If the promise is rejected, the custom logger is not called. I see in the console:

Unhandled promise rejection ReferenceError: invalid_function is not defined
at http://.../someService.js:113:15
at new b
at someCustomElement.attached
at BehaviorInstance.attached
at View.attached
at ViewSlot.attached
at View.attached
(anonymous function) @ shim.min.js:1444
b.exports @ shim.min.js:453
b.(anonymous function) @ shim.min.js:1625
f @ shim.min.js:1596
q @ shim.min.js:1600

I don't know if a workaround exists for this case. It seems like onunhandledrejection may be an eventual solution (https://developer.mozilla.org/en-US/docs/Web/API/Window/onunhandledrejection)

@sylvain-hamel
Copy link
Author

Update to my previous post.

Another variation of case 2 is with a failed XHR Request, I was able to implement a workaround using https://github.com/slorber/ajax-interceptor which looks like this.

AjaxInterceptor.addResponseCallback((xhr)=> {
  if (xhr.status === 500) {
   ...
  }
});
AjaxInterceptor.wire();

I still have not found a workaround for Promises.

Until I get Aurelia's team's point of view on the issue, I keep the two workarounds inside my custom log appender.


The full code is this:

import {EventAggregator} from 'aurelia-event-aggregator';
import {inject} from 'aurelia-framework';
import AjaxInterceptor from 'ajax-interceptor'

@inject(EventAggregator)
export class UnhandledErrorPublisher {
  constructor(eventAggregator) {
    this.eventAggregator = eventAggregator;

    window.addEventListener('error', (errorEvent)=> {
      let msg = `${errorEvent.error.message} \r ${errorEvent.error.stack}`
      this.eventAggregator.publish('Unhandled-Error', msg);
    });

    AjaxInterceptor.addResponseCallback((xhr)=> {
      if (xhr.status === 500) {
        let msg = `${xhr.statusCode} - ${xhr.statusText} \r ${xhr.responseText}`
        this.eventAggregator.publish('Unhandled-Error', msg);
      }
      if (xhr.status === 0) {
        let msg = `XMLHttpRequest request cancelled by browser (status code 0). See console for details.`
        this.eventAggregator.publish('Unhandled-Error', msg);
      }
    });
    AjaxInterceptor.wire();
  }

  error(logger, message, ...rest) {
    this.eventAggregator.publish('Unhandled-Error', message, ...rest);
  }

  debug(logger, message, ...rest) {
  }

  info(logger, message, ...rest) {
  }

  warn(logger, message, ...rest) {
  }
}

which is wired-up this way:

import * as TheLogManager from 'aurelia-logging';
import {UnhandledErrorPublisher} from './unhandled-error-publisher.js'

export function configure(aurelia) {
   TheLogManager.addAppender(aurelia.container.get(UnhandledErrorPublisher));
  TheLogManager.setLevel(TheLogManager.logLevel.debug);

  aurelia.use
    .standardConfiguration()
    .developmentLogging();
  aurelia.start().then(a => a.setRoot());
}

I have another component that subscribes to the Unhandled-Error message and either shows the error in the view in a red panel or tries to send the error to a logging service on the backend (depending on whether or not my app is running in a debug or release configuration).

@Roustalski
Copy link

@sylvain-hamel Great workaround setup. I've also successfully implemented your workaround.

I currently have an existing application in angular, and during the bootstrap of Aurelia, I'm loading a main.js with a configure function. Is there a way to get at the event aggregator when Aurelia starts and subscribe to the event there? How would I load a viewless module, say a simple log implementation, for the uncaught exceptions?

@sylvain-hamel
Copy link
Author

Get EventAggregator from the container:

import {EventAggregator} from 'aurelia-event-aggregator';

export function configure(aurelia) {
 let ea = aurelia.container.get(EventAggregator);
}

@sylvain-hamel
Copy link
Author

I figured out how to handle unhanded promise rejections:

    // handle core-js Promise rejection
    let baseOnunhandledrejection = window.onunhandledrejection;
    window.onunhandledrejection = (rejection)=>{
      let msg = `Unhandled promise rejection : ${rejection.reason}`;
      if (rejection.reason.stack) {
        msg += `\r${rejection.reason.stack}`;
      }

      this.eventAggregator.publish('unhandled-error', msg);

      if (baseOnunhandledrejection){
        baseOnunhandledrejection(data);
      }
    };

The full code is now:

import {EventAggregator} from 'aurelia-event-aggregator';
import {inject} from 'aurelia-framework';
import AjaxInterceptor from 'ajax-interceptor'

@inject(EventAggregator)
export class UnhandledErrorPublisher {
  constructor(eventAggregator) {
    this.eventAggregator = eventAggregator;

    window.addEventListener('error', (errorEvent)=> {
      let msg = `${errorEvent.error.message} \r ${errorEvent.error.stack}`
      this.eventAggregator.publish('Unhandled-Error', msg);
    });

    AjaxInterceptor.addResponseCallback((xhr)=> {
      if (xhr.status === 500) {
        let msg = `${xhr.statusCode} - ${xhr.statusText} \r ${xhr.responseText}`
        this.eventAggregator.publish('Unhandled-Error', msg);
      }
      if (xhr.status === 0) {
        let msg = `XMLHttpRequest request cancelled by browser (status code 0). See console for details.`
        this.eventAggregator.publish('Unhandled-Error', msg);
      }
    });
    AjaxInterceptor.wire();
  }

  // handle core-js Promise rejection
  let baseOnunhandledrejection = window.onunhandledrejection;
  window.onunhandledrejection = (rejection)=>{
    let msg = `Unhandled promise rejection : ${rejection.reason}`;
    if (rejection.reason.stack) {
      msg += `\r${rejection.reason.stack}`;
    }

    this.eventAggregator.publish('unhandled-error', msg);

    if (baseOnunhandledrejection){
      baseOnunhandledrejection(data);
    }
  };

  error(logger, message, ...rest) {
    this.eventAggregator.publish('Unhandled-Error', message, ...rest);
  }

  debug(logger, message, ...rest) {
  }

  info(logger, message, ...rest) {
  }

  warn(logger, message, ...rest) {
  }
}

According to the core-js doc, you also need this in index.html. Even without deleting the native window.Promise it worked with my test case but I guess it is safer to follow the recommendation.

    <script>
      // see https://github.com/zloirock/core-js
      // section : Unhandled rejection tracking
      // warning: If someone wanna use this hook everywhere - he should delete window.Promise before inclusion core-js.
      delete window.Promise;
    </script>
    <script src="jspm_packages/system.js"></script>
    <script src="config.js"></script>
    ...

@stevies
Copy link

stevies commented Jan 21, 2016

Is there a way to just let the error bubble up so code I have in the head of index.html page catches it: eg

window.onerror = function(msg, url, line, col, error) {
  debugger;
};

That's a trivial example. In reality, I have a custom logger in there that deals with posting back to the server. I know I could probably pull all of that code into a custom appender - like solution above - but I don't really want to have to do that. A simple way to turn off aurelia logging altogether would be good, or at least let errors bubble up and not be swallowed.
I tried - in a custom appender:

error(logger, ...rest) {
  debugger; // eslint-disable-line
  throw new Error(...rest);
}

Stops on debugger line OK - but the new thrown error does not get caught by the page. Is there an easy way around that?

@sylvain-hamel
Copy link
Author

@stevies no it's not possible because unhandled ajax errors and unhandled promise rejections don't end up in window.onerror (and that is not related to Aurelia).

I created this issue because I think all Aurelia applications should benefit from a simple and reliable global error handling mechanism and I hope the Aurelia team integrates this into the framework.

@EisenbergEffect
Copy link
Contributor

We could add something like this through our pal abstraction. @sylvain-hamel Would you be willing to put together a prototype?

@sylvain-hamel
Copy link
Author

@EisenbergEffect would you be ok with adding a dependency on AjaxInterceptor (an external library) and one on window.onunhandledrejection (a non standard feature added by core-js)?

@EisenbergEffect
Copy link
Contributor

No 3rd party dependencies please. Those constantly cause us issues. Also, the AjaxInterceptor isn't going to work with Fetch I imagine, which is our preferred mechanism for HTTP.

@sylvain-hamel
Copy link
Author

You are right.

I feel like the core team is in a better position to find the proper implementation and all the right hooks. My current handler does not work if the error occurs too early and does not cover all error types yet. I think it's important that the global error handling mechanism kicks in as early as possible because errors during initial load are common. Our current app (that we converting to Aurelia) can even handle script loading errors (using <script onerror="myHandler()">). I don't know if SystemJS offers script load error handlers.

I can keep sharing code as I find ways to handle things but I think you guys should write the final code.

@EisenbergEffect
Copy link
Contributor

Please keep the ideas coming :)

1 similar comment
@EisenbergEffect
Copy link
Contributor

Please keep the ideas coming :)

@Roustalski
Copy link

My usage of the AjaxInterceptor breaks in typescript, so I manually created a definitions file for anyone who is interested until the official support without the 3rd party becomes available:

./typings/ajax-interceptor.d.ts

declare module 'slorber/ajax-interceptor' {
    export function addRequestCallback(callback: (xhr: XMLHttpRequest) => void): void;
    export function removeRequestCallback(callback: (xhr: XMLHttpRequest) => void): void;
    export function addResponseCallback(callback: (xhr: XMLHttpRequest) => void): void;
    export function removeResponseCallback(callback: (xhr: XMLHttpRequest) => void): void;
    export function wire(): void;
    export function unwire(): void;
}

@sensedeep
Copy link

Another example of an uncaught error in the latest release.

<form submit.delegate="login()">
login() {
    throw new Error('boom')
}

The error is captured and discarded. Nothing in the console or dev tools. Nothing to the Aurelia log either.

@EisenbergEffect
Copy link
Contributor

@sensedeep I just put an error in the submit method of the skeleton. When I click the button or hit enter in the form, I see the error reported in the console clearly. This is on master of all repos, so perhaps it's fixed. However, I don't remember any particular fixes related to this. Could there be something else unique about your scenario?

@sensedeep
Copy link

The difference may be I'm using async functions with the Babel runtime. The full code is:

async login() {
    //     throw new Error('boom')
    let result = await somePromisedEvent
}

I've singled stepped through the code and if I uncomment the "throw", the Babel runtime gobbles the event as it unwinds the async promises. When it returns to evaluate in aurelia-binding, the exception is gone. I can do a try/catch in login(), but I'd like to have a global capture of such exceptions as well. Any ideas? Is there a recommended pattern for exceptions with async functions?

@EisenbergEffect
Copy link
Contributor

Ok, this is a specific problem with the fact that the method is an async method.
@jdanyow What do you think of this? Should we be doing anything to handle this in the event manager? Should we be detecting if a Promise is returned from the method and connecting a "catch" handler to it? I'm not sure...

@stoffeastrom
Copy link

There are some discussions in whatwg here.

Seems like the common thing for vendors would be to call window.onerror eventually. Depending on Promise implementation there are some support already. Not sure if it's feasible to handle all different scenarios.

@EisenbergEffect
Copy link
Contributor

Right. Doesn't Bluebird handle this automatically? Perhaps the best solution would be to just use that?

@stoffeastrom
Copy link

Yeah there are a couple of different approaches for bluebird depending on if it's in node or browser but there seems to be events for this. Haven't used that myself though.

@sensedeep
Copy link

Async functions are really, REALLY elegant. I'm not using bluebird and running in the browser ... chrome only at this point.

To handle this, I think the framework would have to test if a rejected promise is returned. That is what an async function that throws will do.

Ideally, you want the Aurelia logging to capture it at the actual function invoked and not at the bubble up point. Much better for debugging.

@EisenbergEffect
Copy link
Contributor

Closing this as its mostly specific to how the end user is handling promises and ajax.

@atsu85
Copy link
Contributor

atsu85 commented Aug 8, 2016

@sylvain-hamel, thanks for sharing Your solution.

I have a suggestion for imprvemen

Handing errors from Promises used by router (Promises returned by ViewHooks: canActivate, activate, canDeactivate, deactivate) isn't covered by Your solution - I solved it as follows:

    constructor(eventAggregator: EventAggregator) {
        eventAggregator.subscribe("router:navigation:error", (data: RouterNavigationErrorEventData, eventName: string) => {
            const error: Error = data.result.output; // what was rejected - technically it could be smth other than Error, but that would be reported by bluebird as bad practice anyway
            // do smth with the error
        });
    }

where RouterNavigationErrorEventData is alias for

type RouterNavigationErrorEventData = {
    instruction: NavigationInstruction;
    result: PipelineResult;
}

@sylvain-hamel
Copy link
Author

@atsu85 Thanks for your contribution! I'll add this to my project.

@antonmos
Copy link

With all due respect, I do not understand why this issue was closed.
The need to present a clean UX to the user when an unhandled error occurs is a necessity in every single app as is the need to log the cause.
@EisenbergEffect could please clarify?

@zchpit
Copy link

zchpit commented Dec 5, 2016

What is a correct way to global error handling?

I have made my own custom logger from: https://stackoverflow.com/questions/37791068/send-aurelia-error-logs-to-server-and-notify-the-user

But when I throw exception in any part of application, in example:
var a = 2;
if (a == 2) {
throw "catch me if you can";
}

My custom logger don't catch it.

@antonmos
Copy link

antonmos commented Dec 5, 2016

@zchpit see window.addEventListener('error'...) in #174 (comment)

@zchpit
Copy link

zchpit commented Dec 5, 2016

@antonmos I have problem with adding:
import AjaxInterceptor from 'ajax-interceptor';

Cannot find module 'ajax-interceptor'.

I'm using Visual Studio 2015 Update3.
I have updated project.json and node_modules was download to node_modules catalog
I have updated map property in config.js (in same catalog as Aurelias main.ts)

What I'm doing wrong? Or what I need to do, to load that file manualy (in example copy-paste https://github.com/slorber/ajax-interceptor/blob/master/index.js into my solution and load it directly).

@zchpit
Copy link

zchpit commented Dec 6, 2016

Another question:
Doescore-js is in Aurelia by default or I need to install that package as external tool ?

@EisenbergEffect
Copy link
Contributor

We have our own set of polyfills that are the minimal required set. If you prefer core-js, you can install that instead. It isn't required.

@zchpit
Copy link

zchpit commented Dec 6, 2016

@EisenbergEffect I don't have any preferences. I want to have one global custom error handler, that will log all my unexpected errors into my server.

At this moment, I don't have it.
I have something like this:

import 'fetch';//IE Polyfill
import {HttpClient, HttpClientConfiguration, json} from 'aurelia-fetch-client';
import {EventAggregator} from 'aurelia-event-aggregator';
import {BaseService} from '../infrastructure/BaseService';
import {inject} from 'aurelia-framework';
import AjaxInterceptor from 'ajax-interceptor';

@Inject(HttpClient,EventAggregator)
export class CustomLogAppender extends BaseService
{
httpclient: HttpClient;
eventAggregator: EventAggregator;
constructor(http, eventAggregator) {
super(http);
this.eventAggregator = eventAggregator;

    window.addEventListener('error', (errorEvent) => {
        //let msg = `${errorEvent.error.message} \r ${errorEvent.error.stack}`
        let msg = errorEvent.returnValue;
        this.eventAggregator.publish('Unhandled-Error', msg);
    });

    AjaxInterceptor.addResponseCallback((xhr) => {
        if (xhr.status === 500) {
            let msg = `${xhr.status} - ${xhr.statusText} \r ${xhr.responseText}`
            this.eventAggregator.publish('Unhandled-Error', msg);
        }
        if (xhr.status === 0) {
            let msg = `XMLHttpRequest request cancelled by browser (status code 0). See console for details.`
            this.eventAggregator.publish('Unhandled-Error', msg);
        }
    });
    AjaxInterceptor.wire();
}

//Property 'onunhandledrejection' does not exist on type 'Window'
/*
let baseOnunhandledrejection = window.onunhandledrejection;
window.onunhandledrejection = (rejection) => {
    let msg = `Unhandled promise rejection : ${rejection.reason}`;
    if (rejection.reason.stack) {
        msg += `\r${rejection.reason.stack}`;
    }

    this.eventAggregator.publish('unhandled-error', msg);

    if (baseOnunhandledrejection) {
        baseOnunhandledrejection(data);
    }
};*/


debug(logger, message, ...rest) {
    let comment = 'DEBUG ' + logger.id + ' ' + message;
    console.debug(comment, ...rest);

    return new Promise((resolve, reject) => {
        //var url = '/logger/logger/LogDebug';
        var url = 'logger/logger/LogDebug';

        this.http.fetch(url, { method: 'post', body: json(comment) }).then(res => {
            if (res.ok) {
                /*
                res.json().then(data => {
                    return resolve(data);
                });*/
            }
            else {
                reject(res.status);
            }
        });
    });
}

info(logger, message, ...rest) {
    let comment = 'INFO ' + logger.id + ' ' + message;
    console.info(comment, ...rest);

    return new Promise((resolve, reject) => {
        //var url = '/logger/logger/LogInfo';
        var url = 'logger/logger/LogInfo';

        this.http.fetch(url, { method: 'post', body: json(comment) }).then(res => {
            if (res.ok) {
                /*
                res.json().then(data => {
                    return resolve(data);
                });*/
            }
            else {
                reject(res.status);
            }
        });
    });
}
warn(logger, message, ...rest) {
    let comment = 'WARN ' + logger.id + ' ' + message;
    console.warn(comment, ...rest); 

    return new Promise((resolve, reject) => {
        //var url = '/logger/logger/LogWarn';
        var url = 'logger/logger/LogWarn';

        this.http.fetch(url, { method: 'post', body: json(comment) }).then(res => {
            if (res.ok) {
                /*
                res.json().then(data => {
                    return resolve(data);
                });*/
            }
            else {
                reject(res.status);
            }
        });
    });
}
error(logger, message, ...rest) {
    let comment = 'ERROR ' + logger.id + ' ' + message;
    console.error(comment, ...rest);

    return new Promise((resolve, reject) => {
        //var url = '/logger/logger/LogError';
        var url = 'logger/logger/LogError';

        this.http.fetch(url, { method: 'post', body: json(comment) }).then(res => {
            if (res.ok) {
                /*
                res.json().then(data => {
                    return resolve(data);
                });*/
            }
            else {
                reject(res.status);
            }
        });
    });
}

}

@MortenChristiansen
Copy link

@EisenbergEffect Am I understanding it correctly, that to add global logging for async event handlers, I need to use a Promise library? Note - I'm using Typescript compiling to es6 - I don't know if my situation/solution applies in all cases.

I need global error logging and the only way I have found that allows me to capture errors in async is to override the generated __awaiter object inside every view model that uses async event handlers. This is very ugly and cumbersome, but I don't want to add another dependency for such a simple thing. Obviously, I have a method for generating the awaiter function, but that hardly makes the solution awesome.

var __awaiter = (thisArg, _arguments, P, generator) => {
    return new (P || (P = Promise))(function (resolve, reject) {
        function fulfilled(value) { try { step(generator.next(value)); } catch (e) { 
            // Add loggin here
            reject(e); } }
        function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
        function step(result) { result.done ? resolve(result.value) : new P(function (resolve) { resolve(result.value); }).then(fulfilled, rejected); }
        step((generator = generator.apply(thisArg, _arguments || [])).next());
    });
};

export class MyViewModel {
    async buttonClicked() {
        await doStuff();
        throw new Error('Oh noes!!!');
    }
}

If there is a way to do this automatically for all view models, I would be interested to know about it, but it's still not great to replace generated code that could change with future versions of typescript.

@EisenbergEffect
Copy link
Contributor

Check this SO article out and see if it helps: https://stackoverflow.com/questions/28001722/how-to-catch-uncaught-exception-in-promise

@MortenChristiansen
Copy link

@EisenbergEffect It does not. While window.addEventListener("unhandledrejection" does catch some cases, it does not work for this async/await case.

@EisenbergEffect
Copy link
Contributor

This might be something you want to bring up with the TypeScript team, particularly if it's related to the code they are generating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants