Async getHost support, Node 12 deprecation fix #420
Conversation
bconnorwhite
changed the title
bconnorwhite
changed the title
| container.proxy.reqBuilder.port = container.options.port || parsedHost.port; | ||
| container.proxy.requestModule = parsedHost.module; | ||
| return Promise.resolve(container); | ||
| return Promise.resolve(parsedHost).then((parsedHost) => { |
There was a problem hiding this comment.
Thank you for using a promise here for consistency
|
Hey @connorwhite, Thank you for this. I would accept everything excluding the asynchronous host immediately, but I'd like to discuss that part. Historically, this library has held that dynamic host resolution is best handled by the main application, but I'm always interested in discussing this. Can you tell me about the use case that you are solving for? |
|
@monkpow, example use case for asynchronous host would be authenticating a user, and then using different host based on the result, ex: different host for different user roles. Mostly though it just seemed odd to me that most of the other functions support promises but not the host. Is there a reason dynamic host resolution is different from dynamic path resolution (proxyReqPathResolver)? Currently, since proxyReqPathResolver supports promises you could put these at different paths (ex: /endpoint/role1 and /endpoint/role2). Asynchronous host resolution would be nice as it would allow you to also route to different hosts (ex: localhost:4000/endpoint and localhost:4010/endpoint). Also it seems like others have interest in this: |
|
Right on.
I picked several of your commits to master already (related to Node 12 deprecation). I'd like to put this async host on on a separate release. Give me a couple days and I'll come back for the aysnc host commits. |
|
@connorwhite Ok, so I'm closing this one, and opened a new PR over here #433 with just the async host stuff. Thanks again. |
Asynchronous "getHost" implementation:
Issue #284
OutgoingMessage.prototype._headers is deprecated in Node.js 12:
Issue #413