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

Explicit Error classes #83

Closed
floatdrop opened this issue Jul 16, 2015 · 8 comments
Closed

Explicit Error classes #83

floatdrop opened this issue Jul 16, 2015 · 8 comments
Milestone

Comments

@floatdrop
Copy link
Contributor

Before major release happen - maybe we should subclass error a little bit more, than GotError? For example we have parsing errors, http request errors, maxredirects errors, etc...

Sometimes it is useful to differentiate them by class, not by message.

One more concern is internal structure of errors - should we mimic error properties from Node 0.12 by default?

//cc @sindresorhus @kevva @julien-f


Related http://dailyjs.com/2014/01/30/exception-error/

@floatdrop floatdrop changed the title More error classes More Error classes Jul 16, 2015
@floatdrop
Copy link
Contributor Author

Something like this:

diff --git a/index.js b/index.js
index 44f453a..b4bafb6 100644
--- a/index.js
+++ b/index.js
@@ -41,7 +32,7 @@ function requestAsEventEmitter(opts) {
                res.resume();

                if (++redirectCount > 10) {
-                   ee.emit('error', new GotError('Redirected 10 times. Aborting.'), undefined, res);
+                   ee.emit('error', new MaxRedirectsError(statusCode, {url: url}), undefined, res);
                    return;
                }

@@ -70,7 +61,7 @@ function requestAsEventEmitter(opts) {

            ee.emit('response', res);
        }).once('error', function (err) {
-           ee.emit('error', new GotError('Request to ' + url + ' failed', err));
+           ee.emit('error', new RequestError(err.message, {url: url, method: opts.method}));
        });

        if (opts.timeout) {
@@ -101,22 +92,21 @@ function asCallback(opts, cb) {
    ee.on('response', function (res) {
        readAllStream(res, opts.encoding, function (err, data) {
            if (err) {
-               cb(new GotError('Reading ' + url + ' response failed', err), null, res);
+               cb(new ReadError(err.message, {url: url}), null, res);
                return;
            }

            var statusCode = res.statusCode;

            if (statusCode < 200 || statusCode > 299) {
-               err = new GotError(opts.method + ' ' + url + ' response code is ' + statusCode + ' (' + http.STATUS_CODES[statusCode] + ')', err);
-               err.code = statusCode;
+               err = new HTTPError(statusCode, {url: url, method: opts.method});
            }

            if (opts.json && statusCode !== 204) {
                try {
                    data = JSON.parse(data);
                } catch (e) {
-                   err = new GotError('Parsing ' + url + ' response failed', new GotError(e.message, err));
+                   err = new ParseError(err.message, {url: url});
                }
            }

@sindresorhus
Copy link
Owner

👍

@kevva
Copy link
Contributor

kevva commented Jul 18, 2015

Yup, I like it too. +1 for more clarity.

@floatdrop
Copy link
Contributor Author

Done in 1fe1e9a - please review 😃

@floatdrop floatdrop added this to the 4.0.0 milestone Jul 19, 2015
@floatdrop floatdrop mentioned this issue Jul 23, 2015
@floatdrop
Copy link
Contributor Author

There is one question, that's bothers me: should we store statusCode as code property? It was discussed in #9, but in other places people tend to use statusCode ((https://github.com/trentm/node-bunyan#log-record-fields, https://github.com/jshttp/http-errors).

@sindresorhus
Copy link
Owner

I prefer statusCode as it's a clearer intent.

@sindresorhus
Copy link
Owner

@floatdrop What happened to floatdrop@58bfb20 in the 4.0.0 PR? Did you back out of it?

@floatdrop
Copy link
Contributor Author

@sindresorhus yup, I will send a PR with this soon.

@floatdrop floatdrop changed the title More Error classes Explicit Error classes Jul 24, 2015
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

No branches or pull requests

3 participants