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

Support using a custom http-like module #112

Merged
merged 4 commits into from
Dec 4, 2011
Merged

Conversation

jhs
Copy link
Contributor

@jhs 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
Copy link
Member

mikeal commented Nov 24, 2011

by some people do you mean you :)

@mikeal
Copy link
Member

mikeal commented Nov 24, 2011

this breaks forwards from http to https and vice versa.

@jhs
Copy link
Contributor Author

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

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

mikeal commented Dec 4, 2011

Awesome, looks good.

mikeal added a commit that referenced this pull request Dec 4, 2011
Support using a custom http-like module
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants