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 disableErrorHandler option #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MadLittleMods
Copy link
Member

@MadLittleMods MadLittleMods commented Nov 9, 2016

@trevorah
Copy link
Contributor

It looks like this adds an option to the method that cancels an existing option that came from the constructor.

I think that overriding the error handler would be a better fit, or even just having your code use two clients, one with an error handler and one without.

@MadLittleMods
Copy link
Member Author

@trevorah We would need two clients and this is the route we opted for in our skype (@suprememoocow and I)

@@ -158,7 +158,7 @@ Client.prototype = {
});

/* Custom error handler */
if (this.errorHandler) {
if (!options.disableErrorHandler && this.errorHandler) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but a more flexible approach might be to allow the caller to intercept the error before the global error handler kicks in.

So, something like:

// Install local error handler
if (options.errorHandler) {
  promise = promise.catch(options.errorHandler);
}

// Install global error handler
if (this.errorHandler) {
  promise = promise.catch(this.errorHandler);
}

return promise;

This way, you get given the option to swallow the error locally before the global error handler gets called. So, this gives you flexibility to ignore the error, conditionally ignore certain errors or throw a different type of error.

@MadLittleMods MadLittleMods force-pushed the feature/disable-error-handler branch from 627cd11 to 91a27c7 Compare November 16, 2016 15:49
@coveralls
Copy link

coveralls commented Nov 16, 2016

Coverage Status

Coverage decreased (-0.4%) to 95.133% when pulling 91a27c7 on feature/disable-error-handler into f9d24bc on master.

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

Successfully merging this pull request may close these issues.

4 participants