Support using a custom http-like module #112

Merged
merged 4 commits into from Dec 4, 2011

Projects

None yet

2 participants

@jhs
Contributor
jhs commented Nov 24, 2011

Some people may have an API-compatible module that works like http, with custom behavior or a monkey patch or whatever.

This one-liner allows the caller to provide that module in the options.

var my_http = require('./my_http_module')

request({uri:"http://example.com/", httpModule:my_http}, function(er, resp, body) {
  // ...
})

This strikes me as an obscure or advanced feature, so I skipped documentation and tests :) Let me know if you want some and I'll slog through it.

@mikeal
Member
mikeal commented Nov 24, 2011

by some people do you mean you :)

@mikeal
Member
mikeal commented Nov 24, 2011

this breaks forwards from http to https and vice versa.

@jhs
Contributor
jhs commented Nov 24, 2011

Yes, some people means me. But it serves reasonable and general purpose, with no additional lines of code.

I don't see a failing unit test. What forwards from http and https is it breaking? If I can attach tests confirming this feature and also that the forwards don't break, you think you could merge it then?

@mikeal
Member
mikeal commented Nov 28, 2011

here's the issue.

we set this.httpModule to the one we are going to use. before, we didn't check if it was already defined and blew it away each time.

during an HTTP forward the request object is recycled so httpModule will already be set, so if we forward from http to https it will fail.

the only API i could see that might survive a forward would be passing in an alternate map of http modules for the http/https check.

@jhs
Contributor
jhs commented Dec 4, 2011

Weird, once I pushed new work, it all became associated with this pull requests. Not sure how I feel about that.

If I'm reading this correctly, the new changes include three things:

  1. Redirection tests, for 301 and 302 responses (no change to the main code)
  2. Tests for fetching over HTTPS (no change to the main code)
  3. Your suggestion of an alternate map of http/https modules, with tests. The tests redirect between http and https servers, with both default and custom http/https modules.
@mikeal
Member
mikeal commented Dec 4, 2011

Awesome, looks good.

@mikeal mikeal merged commit 550036e into request:master Dec 4, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment