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

errors logged to console that could cause confusion #13

Closed
jchip opened this issue Nov 22, 2013 · 8 comments
Closed

errors logged to console that could cause confusion #13

jchip opened this issue Nov 22, 2013 · 8 comments
Assignees

Comments

@jchip
Copy link

jchip commented Nov 22, 2013

I observed some error and stack trace logged to the console that was puzzling.

Per Erik:

Found it.

It's coming from the default error handler in kraken-js: https://github.com/paypal/kraken-js/blob/master/lib/middleware/error.js#L49

The error is being handled by express, thus it's technically not "uncaught".

Can you file a bug against kraken-js to improve this? Thanks,

-Erik

@ghost ghost assigned jeffharrell Nov 23, 2013
@pvenkatakrishnan
Copy link
Member

@jeffharrell if you have not gotten to this yet, i can take it.

@jeffharrell
Copy link
Member

I haven't yet. That would be great!

-- Jeff

On Dec 2, 2013, at 6:32 PM, "pvenkatakrishnan" <notifications@github.commailto:notifications@github.com> wrote:

@jeffharrellhttps://github.com/jeffharrell if you have not gotten to this yet, i can take it.


Reply to this email directly or view it on GitHubhttps://github.com//issues/13#issuecomment-29678630.

@pvenkatakrishnan
Copy link
Member

@totherik Currently we pass the error back in the model, as is... Did we want to do something fancy for logging in the error handler ? or were you thinking improvements for what is passed in the model ? it will help to get more insight into what you were thinking.

@totherik
Copy link
Member

totherik commented Dec 5, 2013

we need to log the error handler, but I don't have a solution for that just yet. the confusion was that it was logging to the console. one solution would be for kraken to emit server error events and allow app to listen to them to deal with logging message/errors.

@pvenkatakrishnan
Copy link
Member

@pvenkatakrishnan
Copy link
Member

i just noticed something in the EventEmitter class within node. There is a default 'error' event handler within. SO in my solution above, iff the app decides not to handle errors, and kraken emits the 'error' event anyway, it will thrown by the EventEmitter class which is not desirable in our case. App should have the flexibility of ignoring errors if they dont want to look at it.
SO the default event name for errors should be something other than 'error' :)

  // If there is no 'error' event listener then throw.
  if (type === 'error' && !this._events.error) {
    er = arguments[1];
    if (this.domain) {
      if (!er)
        er = new Error('Uncaught, unspecified "error" event.');
      er.domainEmitter = this;
      er.domain = this.domain;
      er.domainThrown = false;
      this.domain.emit('error', er);
    } else if (er instanceof Error) {
      throw er; // Unhandled 'error' event
    } else {
      throw Error('Uncaught, unspecified "error" event.');
    }
    return false;
  }

@pvenkatakrishnan
Copy link
Member

The code above is from the node core, EventEmitter class

t0lkman pushed a commit to t0lkman/kraken-js that referenced this issue Feb 6, 2014
t0lkman pushed a commit to t0lkman/kraken-js that referenced this issue Feb 6, 2014
add config option to specify app route
@pvenkatakrishnan
Copy link
Member

This will be fixed with Kraken-v1.0.0 closing this out.

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

No branches or pull requests

4 participants