request/request-promise

HEAD call useless without resolveWithFullResponse

zcei opened this issue · 8 comments

zcei commented

As a HEAD call is a GET call without response body, it's pretty useless without the resolveWithFullResponse option, as you get an empty string all the time.

I personally would vote for having res.headers as the default parameter passed to .head() calls, or to at least set resolveWithFullResponse to true by default.

Or is it intended to preserve the same behavior in all convenience functions, even if it's nonsense to a particular function?

Good point @zcei !

Indeed, I do prefer having the same default settings for all convenience functions because it is less confusing for the users. Nonetheless could you rephrase your proposed change? I don't understand what you mean by "having res.headers as the default parameter passed to .head() calls".

Right away you may make use of the .defaults() feature:

var rp = require('request-promise');
var rpWithFullResp = rp.defaults({ resolveWithFullResponse: true });

rpWithFullResp.head(...);
rp.get(...);
zcei commented

Uh, saw the defaults but didn't read properly. I thought it'd set it on rp, thanks!
That said, it's actually sufficient for me, consistency has a lot of value worth keeping.

The proposed change would be one .then() function as an interceptor for HEAD calls.
e.g.:

.then(function extractHeader(res) {
  return res.headers;
})

I like your idea. This improves convenience and doesn't really introduce inconsistency in this case.

zcei commented

Would it be sufficient to add another else if branch?

    if (isFunction(self._rp_options.transform)) {
        try {
            self._rp_resolve(self._rp_options.transform(body, response));
        } catch (e) {
            self._rp_reject(e);
        }
    } else if (self._rp_options.resolveWithFullResponse) {
        self._rp_resolve(response);
+   } else if (response.request.method === 'HEAD') {
+       self._rp_resolve(response.headers);
    } else {
        self._rp_resolve(body);
    }

or would you directly want to implement something like RP$middlewareInterceptor, to possibly treat other special cases?

I already started the implementation and chose to add it directly because that should be much more performant:

    if (isFunction(self._rp_options.transform)) {
        try {
            self._rp_resolve(self._rp_options.transform(body, response));
        } catch (e) {
            self._rp_reject(e);
        }
    } else if (self._rp_options.resolveWithFullResponse) {
        self._rp_resolve(response);
    } else {
-       self._rp_resolve(body);
+       self._rp_resolve(self._rp_options.method && self._rp_options.method.toUpperCase() === 'HEAD' ? response.headers : body);
    }

However, an interceptor might be a very useful idea. Please have a look at the transform function and give me feedback whether this already covering everything you imagine or if we should extend its capabilities or if a new interceptor mechanism would allow to cover new use cases. Thanks a lot!

zcei commented

Transform seems fine for me.
Then we could modify the RP$initInterceptor:

    if (isString(options.method)) {
        options.method = options.method.toUpperCase();
+       options.transform = options.transform || defaultTransformations[options.method];
    }

Obviously with an defaultTransformations object like:

var defaultTransformations = {
    HEAD: extractHeaders
};

Then the transformation stays undefined as it currently is, when we don't need any further treatment.

Thanks @zcei that is a really clean solution!
I will take it from here and let you know when I released the next version.

I just released version 1.0.0 which fixes this issue. Thanks for providing the solution in your last comment @zcei which I just had to apply.