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

HEAD call useless without resolveWithFullResponse #58

Closed
zcei opened this issue Aug 27, 2015 · 8 comments
Closed

HEAD call useless without resolveWithFullResponse #58

zcei opened this issue Aug 27, 2015 · 8 comments

Comments

@zcei
Copy link

zcei commented Aug 27, 2015

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?

@analog-nico
Copy link
Member

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
Copy link
Author

zcei commented Aug 27, 2015

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;
})

@analog-nico
Copy link
Member

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

@zcei
Copy link
Author

zcei commented Sep 2, 2015

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?

@analog-nico
Copy link
Member

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
Copy link
Author

zcei commented Sep 2, 2015

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.

@analog-nico
Copy link
Member

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

@analog-nico
Copy link
Member

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.

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

2 participants