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

app error handling improvements #48

Closed
jonathanong opened this issue Sep 7, 2013 · 5 comments
Closed

app error handling improvements #48

jonathanong opened this issue Sep 7, 2013 · 5 comments

Comments

@jonathanong
Copy link
Member

right now, app.onerror is added to the the error event listener automatically. what happens if users don't want to use it? doing app.removeListener('event', app.onerror)` is a little annoying. instead i suggest the following:

  • remove app.onerror
  • remove app.outputErrors
  • at the app.callback() stage, warn/throw if no event listeners have been added
  • automatically add the default listener in development
app.callback = function(){
  // ...

  if (!app.listeners('error')) {
    if (this.env === 'development') {
      console.warn('You should add your own error handler.');
      app.on('error', function(err) {
        console.error(err.stack);
      })
    } else if (this.env !== 'test') {
      throw new Error('Add your own error handler!');
    }
  }

  return function(req, res){
  // ..
})
@tj
Copy link
Member

tj commented Sep 8, 2013

how about we remove app.outputErrors (and just imply that stuff in the default handler), and make it app.error(fn) so that you can easily override it without manually removing one or all or some of the listeners, but at least then it's still "batteries included"

@jonathanong
Copy link
Member Author

something like:

function App() {
  this.on('error', logger);
}

App.prototype.error = function(fn){
  this.removeListener('error', logger);
  this.on('error', fn);
}

function logger(err) {
  if ('test' != this.env) console.error(err.stack);
}

i think that'll work. remember, no .bind(this) necessary in node! haha

@tj
Copy link
Member

tj commented Sep 8, 2013

yeah I think that'll work pretty good, and we can forward mounted app errors upstream

@tj
Copy link
Member

tj commented Sep 8, 2013

think im fine with this for now actually, it's easier to explain app.outputErrors = false than if you call app.error() it overrides it, and you can add additional listeners with "error" etc blha blah. Plus IMHO you always want stderr traces, never know when other forms of reporting may fail

@tj tj closed this as completed Sep 8, 2013
@jonathanong
Copy link
Member Author

haha, so i thought about errors where you don't want them to be logged, but then i realized how much easier error handling will be vs express

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

2 participants