More easily checkable errors

[matthew-andrews]

Would you be open to a pull request that creates more specific errors than just Error. If I remember correctly the whatwg-fetch spec leaves errors up to implementations to decide how to handle so I think we should be free to do what we want…

It would be really helpful if there was something like a FetchError for all errors (although a different type per error would be OK too) so we can do things like

var fetch = require('node-fetch');
var FetchError = require('node-fetch').FetchError;

fetch('https://time.out/error')
  .catch(function(err) {
    if (err instanceof FetchError) {
      // send the failure to our metrics system but don't raise it as a fault
    } else {
      // send the error to our error aggregation system
    }
  });

[bitinn]

I would be ok with that, though I didn't expect fetch to produce any error other than FetchError, do you really think that conditional instanceof is necessary?

[matthew-andrews]

it'd probably go at the end of a big chain of stuff, e.g:-

https://github.com/Financial-Times/next-grumman/blob/master/server/controllers/capi.js#L127

            if (err instanceof fetchres.BadServerResponseError || err instanceof FetchError) {

[bitinn]

Looks like network error is main pain here, per spec though, network error is a response with status 0.

I wonder if we should change behavior according to spec instead: https://fetch.spec.whatwg.org/#concept-network-error

ie. resolve instead of reject on network error (and custom timeout)

[matthew-andrews]

oh that would actually be OK for us assuming .ok would be false in this case?

[bitinn]

yeah ok should be false, though that means the spec need update again?

update: looking at the wording, as long as response is not in range 200 ~ 299, it should be false

[bitinn]

This is incorrect, see follow up comment

I have a 2.0 branch ready for testing, see if covers your need

https://github.com/bitinn/node-fetch/tree/2.0

see test for timeout

https://github.com/bitinn/node-fetch/blob/2.0/test/test.js#L317

[bitinn]

Though I am not exactly sure if TCP timeout would be a problem (node.js net doesn't have a default timeout, so I guess it's up to linux server to decide).

[bitinn]

Sorry, after reading the spec a bit more though, I realize what they mean by Network Error is a Response:

Response here is just an internal object, and Network Error is a Response object with .type set to error (we didn't do that yet, and github polyfill always set it to default).

But per spec, we don't actually resolve Network Error, we should check for type error and then reject it with TypeError. https://fetch.spec.whatwg.org/#fetch-method

This means your problem is still valid, one cannot easily distinguish a custom timeout/maximum redirect errors (currently you have to check error message, which sucks), unless we establish some sorts of convention on how Error will be formatted.

[pekeler]

Yes, checking the error message sucks.

network timeout at: http://10.255.255.1
request to http://10.255.255.1 failed, reason: connect ETIMEDOUT 10.255.255.1:80
request to http://this.defninitelydoesnotexist.ca/ failed, reason: getaddrinfo ENOTFOUND this.defninitelydoesnotexist.ca this.defninitelydoesnotexist.ca:80
maximum redirect reached at: http://localhost:8081/api/test/redirect

The important keywords (timeout, ETIMEDOUT, ENOTFOUND, max-redirect) are all at different positions in the message, which makes parsing more complicated than it could be.

Could we just add a code property to the error object, similar to https://github.com/request/request?

[bitinn]

@pekeler the idea of Fetch is user shouldn't know why it failed (due to security), user should just know that it failed. So our custom error message is really for human troubleshooting, not machine processing. I am not against making it machine readable, but it will most definitely be against Fetch Spec. If you have a better suggestion, please move it to #8 :)

[pekeler]

I just read whatwg/fetch#25 and the security argument makes no sense to me because someone with bad intentions could just use curl or some other networking tool to find out more details.

I'd like to encourage you to focus less on 100% compatibility with an unfinished standard, and more on providing a viable alternative to https://github.com/request/request :)

I like the proposed approach of adding a code property to the error because it is such a limited departure from the standard.

[bitinn]

@pekeler checkout https://github.com/sindresorhus/got if a replacement for request is what you are looking for.

[pekeler]

Thanks for the link @bitinn. I didn't know about got. Unfortunately, looks like got doesn't allow me to control redirects which is important to me.

[bitinn]

@pekeler just so you know, we are still working on that as well, see #98 ... the spec has changed recently to allow end-user to not follow redirect, previously it was internal to browser. Only chrome is known to handle it on client-side and comes with a catch...

[dandv]

One of the listed Features of node-fetch is

explicit errors for troubleshooting

Options also include "node.js extensions".

Can we expose the error perhaps as a node.js extension, and document the difference from the client, as mentioned in the 2nd item under Features?

[bitinn]

By extension we mean it provides parameters beyond the default Fetch Spec (mostly for server-side convenience).

Could you open an issue to clarify when "expose the errors" mean.

[dandv]

Could you open an issue to clarify when "expose the errors" mean.

I believe this very issue shows when @pekeler and I mean: a code property in the error object that has standard errno values such as ETIMEDOUT, ESOCKETTIMEDOUT, ECONNRESET, ECONNREFUSED, EHOSTUNREACH.

Use case: we use the error codes above in node-influx to decide whether to resubmit a failed request or not.

[bitinn]

We had a long discussion on this in the PR and the result is ultimately we want custom error code.

Regards,
David

在 2016年9月25日，14:56，Dan Dascalescu notifications@github.com 写道：

Could you open an issue to clarify when "expose the errors" mean.

I believe this very issue shows when @pekeler and I mean: a code property in the error object that has standard errno values such as ETIMEDOUT, ESOCKETTIMEDOUT, ECONNRESET, ECONNREFUSED, EHOSTUNREACH.

Use case: we use the error codes above in node-influx to decide whether to resubmit a failed request or not.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.

[dandv]

The PR lists some error codes, but you changed some of them and I see that system error codes are also passed through, though not under the .type property, but under .code.

Where can we find a complete list of error codes, along with how to distinguish between custom and system error codes/types? Perhaps error handling merits more of a documentation section?

[bitinn]

I should preface this by saying: Fetch Spec isn't designed to be transparent about errors, anything we do will be an extension. And I try to keep it to a minimum.

There are currently no document on a list of possible errors, but:

• System errors from core can be distinguished with err.type === 'system'.
• Hopefully the sources are minimal enough, a simple search of FetchError should yield all custom ones.

In many cases whether or not a request should be re-initiated is entirely based on use-case. So I didn't find providing a detailed README section to be necessary (usually developers run into errors, and decide how to handle them).

And to be honest, the more options you add to a module, the more possible errors it yields. So if you do want to handle errors before they even appear. The safest bet is to go as low-level as possible: http.request.

[bitinn]

I have updated an error handling doc for this purpose in v1.6.3:

https://github.com/bitinn/node-fetch/blob/master/ERROR-HANDLING.md