Adding Promise support for "getHost". #285
Conversation
test/decorateRequest.js
Outdated
| @@ -65,7 +65,7 @@ describe('decorateRequest', function() { | |||
| it('returns err to host application for processing', function(done) { | |||
| var app = express(); | |||
|
|
|||
| app.use(proxy('/reject-promise', { | |||
| app.use(proxy('httpbin.org', { | |||
There was a problem hiding this comment.
Unrelated issue. This test would always fail since host must start with http(s).
There was a problem hiding this comment.
I think this comment is incorrect. Test pass today without all starting with http. Current code assumes http if http/https is not provided.
|
@OronNadiv Thanks for this patch. I'm starting to review now. |
|
|
||
| if (container.options.memoizeHost && container.options.memoizedHost) { | ||
| parsedHost = container.options.memoizedHost; | ||
| promise = Promise.resolve(container.options.memoizedHost); |
There was a problem hiding this comment.
At first glance, this appears to conflict with the goal of memoizing the host.
There was a problem hiding this comment.
I have a case where an external server defines which users should go to server A and which should go to server B. In that case, every incoming request would query that external server. I plan to cache the results (MRU + expiration), but I still need an async support to determine the target host url. Does it make sense?
| port: parsed.port || (ishttps ? 443 : 80), | ||
| module: ishttps ? https : http, | ||
| }; | ||
| host = (typeof host === 'function') ? host(req) : host; |
There was a problem hiding this comment.
dropped toString() here. investigate why.
There was a problem hiding this comment.
I don't think the toString() is needed. Anyways, I moved it to line 35 to support backward compatibility.
|
Thanks for reviewing it @monkpow. I answered a few of your questions. |
OronNadiv
force-pushed
the
promisified-host
branch
from
f648d1a to
080e2bb
Compare
OronNadiv
force-pushed
the
promisified-host
branch
from
080e2bb to
b1c2981
Compare
@monkpow @villadora
This PR adds support for getting hostname asynchronously using Promises. It solves #284.