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

fix: upgrade http-proxy module for bug fixes #1345

Closed
wants to merge 1 commit into from
Closed

fix: upgrade http-proxy module for bug fixes #1345

wants to merge 1 commit into from

Conversation

terinjokes
Copy link
Contributor

The http-proxy module was significantly outdated, and was missing many
bugfixes to support node newer than 0.10, as we as a refactor and
simplification of their codebase.

This commit upgrades the http-proxy dependency, and refactors the proxy
middleware to utilize the new API.

parseProxyConfig (an internal function) now takes the proxy
configuration object and converts it into an array of "ProxyRecord"
objects, sorted by path. A ProxyRecord object contains:

  • host The remote proxy host
  • port The remote proxy port
  • baseUrl The remote proxy path
  • path The local URL path (that triggers the rewrite)
  • https Boolean to determine if the remote connection will be made
    with SSL.
  • proxy The instance of http-proxy.

This change was necessitated by the removal of http-proxy's
RoutingProxy API, and the desire to only create one proxy instance per
configuration item.

The middleware simply determines if the current request matches a known
proxy path, and calls the proxy's .web and .ws methods with the
current request and response objects.

The relevant unit tests were rewritten in light of these changes.

The http-proxy module was significantly outdated, and was missing many
bugfixes to support node newer than 0.10, as we as a refactor and
simplification of their codebase.

This commit upgrades the http-proxy dependency, and refactors the proxy
middleware to utilize the new API.

`parseProxyConfig` (an internal function) now takes the proxy
configuration object and converts it into an array of "ProxyRecord"
objects, sorted by path. A ProxyRecord object contains:

  * `host` The remote proxy host
  * `port` The remote proxy port
  * `baseUrl` The remote proxy path
  * `path` The local URL path (that triggers the rewrite)
  * `https` Boolean to determine if the remote connection will be made
    with SSL.
  * `proxy` The instance of http-proxy.

This change was necessitated by the removal of http-proxy's
RoutingProxy API, and the desire to only create one proxy instance per
configuration item.

The middleware simply determines if the current request matches a known
proxy path, and calls the proxy's `.web` and `.ws` methods with the
current request and response objects.

The relevant unit tests were rewritten in light of these changes.
@megawac
Copy link

megawac commented Mar 12, 2015

big 👍

@dignifiedquire
Copy link
Member

This is great, a big thanks first of all!
There is only one question I have, what about the option proxyValidateSSL, why did you remove it?

@@ -167,7 +167,7 @@
"chokidar": ">=0.8.2",
"glob": "~3.2.7",
"minimatch": "~0.2",
"http-proxy": "~0.10",
"http-proxy": "~1.8.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change this to ^1.11.1 please?

@dignifiedquire
Copy link
Member

ping @terinjokes

@dignifiedquire dignifiedquire self-assigned this Jul 8, 2015
@dignifiedquire dignifiedquire added this to the 0.13 milestone Jul 8, 2015
@terinjokes
Copy link
Contributor Author

@dignifiedquire sorry, i've been busy and didn't check in on this.

I no longer remember why I modified proxyValidateSSL, but I'll rebase this today and try to answer it for you. 👍

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@dignifiedquire
Copy link
Member

Thanks

@dignifiedquire
Copy link
Member

Merged as bcce563

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

4 participants