When reporting an error result.url is not set #15

Closed
karli2000 opened this Issue Jan 9, 2013 · 6 comments

Comments

Projects
None yet
2 participants

Hi,

sorry, another issue :)

When using the HEAD-function of http-get and requesting an URL that returns a error (404, 403, 502, etc.) the result object is not set. For example try this URL:
http://t.co/w0ErWk1f

This resovles to:
http://www.republicof3.com/5-ux-trends-of-2012-continuing-in-2013/

When making a HEAD-request for the t.co URL the resulting URL returns a 403 error, but it would be nice when the result object has at least the last URL that was called, otherwise you don't have a clue what URL was responsible for the error.

Thank you,
Max

Owner

SaltwaterC commented Jan 9, 2013

The result not being set in case of error is by design. Or should I say, discovery. I am keeping a clear convention of passing either the (error) or the (null, result) arguments to the callback. This way you don't have to exercise the muscle memory for remembering that stuff may be in result if error is set. It used to be the other way around when I started http-get, but things got complicated and I refactored those bits.

// the logic doesn't get any easier than this
if (error) {
  // handle error
} else {
  // handle result
}

I'll attach an url property to the error argument if it is set.

Perfect, i can understand the design decision, it completly makes sense to attach the url property to the error argument, this would fix my problem perfectly.

Owner

SaltwaterC commented Jan 9, 2013

$ node
> var http = require('./')
> http.head('http://t.co/w0ErWk1f', function (err, res) {console.log(err, res)})
> { [Error: HTTP Error: 403]
  code: 403,
  headers: 
   { date: 'Wed, 09 Jan 2013 08:11:40 GMT',
     server: 'LiteSpeed',
     connection: 'Keep-Alive',
     'keep-alive': 'timeout=5, max=100' },
  url: 'http://www.republicof3.com/5-ux-trends-of-2012-continuing-in-2013/' } undefined

Seems just about right. I need to check what tests can cover it as there are a lot of exit points due to errors. Implemented it for both GET and HEAD.

Perfect, that is exactly what i need!

Owner

SaltwaterC commented Jan 9, 2013

Release tagged and published to npm. Broke the commit message, therefore have to close this ticket by hand. Please let me know if you encounter any issues with the new functionality.

SaltwaterC closed this Jan 9, 2013

Works perfect, thank you very much!

Max

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