firebase / geofire-js

GeoFire for JavaScript - Realtime location queries with Firebase
MIT License
1.45k stars 345 forks source link

Create global error logger in GeoFireUtils #26

Closed jwngr closed 9 years ago

jwngr commented 10 years ago

With a lot of other Firebase libraries, errors begin with the name of the library, such as "Firebase Simple Login: error message." We should adopt this practice for GeoFire. It will probably be easiest to just make a utility method logError(message) which appends to "GeoFire:" to the error message.

mikepugh commented 10 years ago

It'd be useful for libraries to adopt a standard error format. Typically when I'm writing my apps, I'll have a central location setup to handle exceptions and they could come from any number of external libraries. It'd be nice to have a way to identify the library the exception came from, and then a code to indicate the error. Having to rely on hard coded error strings like "GeoFire: Remove key failed" are prone to mistakes since those strings often change or can get fat fingered on my end.

If instead you threw an object that looked something like:

{
    lib: "GeoFire",
    code: 1000,
    msg: "Remove key failed"
    stack: {..stack trace...},
    data: { some: 'relevant', data: 'here', when: 'applicable' }
}

An object like that along with publishing a set of error codes with the documentation, would make it slightly easier to parse and less prone to error (for example, if I bower update and don't notice that you started adding "GeoFire: " to the beginning of your errors).

// Internal library function for throwing errors
function logError(code, message, data) {
    var err = new Error();
    throw {
        lib: "GeoFire",
        code: code || -1,
        msg: message,
        data: data,
        stack: err.stack
    };
}

// example failure on removing
logError(1000, 'Remove key failed', {key: 'abcdefg'});

Now I could listen for that error somewhere in my app and either respond somehow, or at least log it to some service of mine (a la Sentry) with useful contextual data.

jwngr commented 10 years ago

Hey @mikepugh - sorry, I thought I had responded to this. Thanks for the suggestion! I'm going to bring this up with the whole team and see if we can come up with a common error format for all of our libraries. I'll keep you posted.

jwngr commented 9 years ago

I don't think I ever really got buy-in from the rest of the team on doing this across all of our repos, so I'm closing this. Maybe we will revisit it someday...