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

Create a real error object on bad HTTP response #155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evanp
Copy link
Contributor

@evanp evanp commented Aug 5, 2013

I frequently find myself special-casing the OAuth request error responses, since they are not instances of the Error class.

This change adds a new class, OAuth.Error, and returns it in case of errors in the HTTP requests. It has the same properties as the original Object instance.

@ciaranj
Copy link
Owner

ciaranj commented Aug 5, 2013

Yeah, I have very few regrets generally in life, but not passing back a real error was a definite mistake, I can't even remember why I chose to do that at the time (I do know better, honest)

I guess my only issue would be whether changing it could break dependant behaviour, if someone is depending on 'Error' vs 'Object literal' to determine whether it is a failure in the OAuth library vs a failure in the underlying request (but since both cases are likely equally fatal, I guess the continuation would be the same in eitheer case?)

@jonathanong
Copy link

+1

@1j01
Copy link

1j01 commented May 5, 2015

👍 I wouldn't worry too much about breaking people's theoretical code depending on errors not being Errors. Although since you're already at 0.9.12, you could publish 1.0.0 which would be very strictly semver compliant.

@1j01 1j01 mentioned this pull request May 5, 2015
@radiovisual
Copy link

radiovisual commented Apr 25, 2016

@ciaranj , you don't have to worry about breaking existing behavior if you are careful about your (semver) versioning (afterall, that is what semver is all about). I totally agree that returning actual Errors is something that should be implemented right away. It's a long-awaited feature that is getting ignored (#84, #250, #155).

If anything, the community will thank you for the breaking change if it means giving them something they want! 😄 👍

@radiovisual
Copy link

radiovisual commented Apr 27, 2016

@ciaranj , I wrote a module which I think is a workaround to this problem. Users of the current (and previous) releases of the oauth module can simply install my module via npm, so there is no messing with the current API or breaking changes in the oauth module.

The module I wrote is called node-oauth-error, which takes in the error objects returned from oauth and converts them into actual Error objects (complete with stack traces).

It's as simple as passing the oauth error object into node-oauth-error's constructor, like this:

new OAuthError(oauthErrorObject);

Here is a usage example from the module's readme:

const oauth = require('oauth');
const OAuthError = require('node-oauth-error');

oauth.get('some/url/endpoint',
    credentials.accessToken,
    credentials.accessTokenSecret,
    (err, data) => {
        if (err) {
            // convert the oauth error object into a real `Error()`.
            throw new OAuthError(err);
        }
        // ...
    }
);

Pull requests or suggestions are welcome if you want to take a closer look.

@popomore
Copy link

+1

@ciaranj Do you have any plan to merge this pr?

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

Successfully merging this pull request may close these issues.

6 participants