Reject with an error object

[jakecraige]

I ran into a problem when using this with mocha. The problem is because when rejecting, this library returns a JS object instead of an Error object. I'd like to open up discussion for changing it to return an error object instead. The options can still be put on there for introspection purposes, but I think it makes more sense to reject with a true Error.

From what I can tell, this is where it happens: https://github.com/tyabonil/request-promise/blob/master/lib/rp.js#L48-L58 and you have tests ensuring this is the case here

Issue also reported to mocha: mochajs/mocha#1532

[analog-nico]

Hi @jakecraige its a valid point. However, I would prefer not to break the backwards-compatibility so I would introduce a new option flag with which you could turn this different rejection on.

Thinking outside the box maybe adding a .toString() to the rejection object would also work?

[JSteunou]

@analog-nico I'm sorry but rejecting with an Error Object is the standard way to do.

[tyabonil]

We can extend the error object in the reject. That said, what practical usecases are you being blocked for?  

—
Ty

On Wed, Feb 4, 2015 at 2:27 PM, Nicolai Kamenzky [email protected]
wrote:

Hi @jakecraige its a valid point. However, I would prefer not to break the backwards-compatibility so I would introduce a new option flag with which you could turn this different rejection on.

Thinking outside the box maybe adding a .toString() to the rejection object would also work?

Reply to this email directly or view it on GitHub:
#38 (comment)

[analog-nico]

@JSteunou I am completely with you. However, IMO we also need to consider the existing users who overlook a breaking chance notice when updating. We would actually mess with their error recovery code... Anyway, we should look forward as @tyabonil says.

I would say the following code would not break backward-compatibility since the Error objects have the same properties as the objects in the current implementation:

    if (err) {

        self._rp_reject(_.assign(new RequestError(String(err)), {
            error: err,
            options: self._rp_options,
            response: response
        }));

    } else if (self._rp_options.simple && !(/^2/.test('' + response.statusCode))) {

        self._rp_reject(_.assign(new StatusCodeError(response.statusCode + ' - ' + body), {
            error: body,
            options: self._rp_options,
            response: response,
            statusCode: response.statusCode
        }));

    }

Would that cover your use cases?

[JSteunou]

semver can prevent breaking changes updates.

[analog-nico]

@JSteunou Being realistic people may still overlook breaking changes. And those who don't should not need to migrate a whole lot. I hope my proposed solution can meet all interests. Does it meet yours?

[analog-nico]

@jakecraige Does my proposed solution meet your needs? In the first case Mocha would display the stack trace starting from the code in Request-Promise not the one of the err returned by Request. I hope that is fine.

[jakecraige]

@analog-nico That solution sounds good to me. It would fix the issue I'm seeing and maintain backwards compat 👍

[analog-nico]

I just published version 0.4.0 to npm.

[jakecraige]

Looks great. Thanks for taking care of this @analog-nico !

[analog-nico]

You are welcome. If anything comes up feel free to ping me on Gitter or open another issue.