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

Throw error when making HEAD request with a body #1429

Merged
merged 2 commits into from Feb 16, 2015

Conversation

tikotzky
Copy link
Contributor

Currently it only throws the error if you make the request using request.head(uri)

This PR updates to also throw the error when making the request by manually setting the method

request({
  uri: uri,
  method: 'HEAD'
})

@tikotzky
Copy link
Contributor Author

This change is required in order to fix recursive request.defaults requesters.

I'm waiting until this gets merged before opening that PR since that change needs the changes from this PR.

If you would like to see the changes to fix recursive request.defaults requesters they are here:
tikotzky@e2f2338
tikotzky@6330b9a

@nylen
Copy link
Member

nylen commented Feb 16, 2015

Looks good to me. Thanks!

nylen added a commit that referenced this pull request Feb 16, 2015
Throw error when making HEAD request with a body
@nylen nylen merged commit b8de4a2 into request:master Feb 16, 2015
@tikotzky tikotzky deleted the throw-error-when-head-with-body branch February 17, 2015 20:13
@simov simov mentioned this pull request Mar 29, 2015
@gabmontes
Copy link

@tikotzky if request is called with a callback, it should call the callback with an error instead of throwing to keep the interface consistent. Currently you have to handle errors this way:

try {
  request({
    uri: uri,
    method: 'HEAD'
  }, function (err, callback) {
    // manage standard success and error cases
  })
} catch (err) {
  // manage throw case
}

Wouldn't it be much cleaner to only manage the errors within the callback?

@tikotzky
Copy link
Contributor Author

@gabmontes I don't remember exactly whats going on here.

But i'm pretty sure the reasoning is that if you pass a body to a HEAD request then its an invalid request that cannot be made. Since the lib cannot make the request it throws.

The callback will get called with an error in the case that there was an error while making the request. If you try to make an invalid request the lib will just throw.

@gabmontes
Copy link

Thanks for your quick response, @tikotzky!!!

I just opened #2109 as this should be changed to always call the callback instead of throwing errors. Please consider doing that change as it will clean up the error handling code in user-space.

In the meantime, a quick workaround I implemented is something like:

function safeRequest (options, callback) {
  try {
    request(options, callback);
  } catch (err) {
    callback(err);
  }
}

Thanks again!

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.

None yet

3 participants