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

Proxy path option #439

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Proxy path option #439

wants to merge 5 commits into from

Conversation

Keksike
Copy link

@Keksike Keksike commented Apr 5, 2018

Add new "proxy path" -R option, which you can use to specify a certain path for which the requests are proxied. Other paths are not proxied.

Example:

If I want to proxy only requests done to path /api/, I can set -R /api/. Then:

GET /a.html -> not proxied
GET /api/data -> proxied

Solves issue: #280

@Keksike
Copy link
Author

Keksike commented Apr 5, 2018

Hey,

I guess this will still need some work. It works perfectly fine when testing locally, but on my AWS deployment it somehow breaks.

@Keksike
Copy link
Author

Keksike commented Apr 26, 2018

Actually I guess it works, issue is probably just with my AWS setup :)

@BigBlueHat
Copy link
Member

Thanks for coding this idea!

First, some minor house keeping:

  • squash the current commits with focused commit messages
  • write tests!

Second, we'd need to address some of the confusion around the semantics of the command and the likely request for doing this with multiple paths. URL rewriting has come up before and #377 explores a "config-based" approach. However, keeping http-server "config-less" (or command line switches only) is also very appealing.

Consequently, things could get confusing when folks try to figure out how to use this feature. For instance:

  • http-server --proxy http://api.example/ - works now; everything that isn't on disk goes here
  • http-server --proxy http://api.example/ -R /api/ - this PR; only things not on disk and that contain this prefix
  • http-server -R /api/ - would fail; should it throw an error? just get ignored?
  • http-server --proxy http://api.example/ -R /api/ -R /a/ - not sure what would/should happen here...
  • http-server --proxy http://api.example/ -R /api/ --proxy http://db.example/-R /db/ - ...someone will ask for this...

Currently, the --proxy option will catch everything not on disk...and I can understand not wanting that to happen. However, because of the scenarios mentioned above, providing this -R option (which might need renaming...) would sadly add confusion, not clarity. But, I think this has merit! It just needs some more discussion/code to make it sing. 😃

There are bound to be other servers out there who handle a "path + URL" mapping from the command line. Perhaps picking up some of their tips and trips may help here.

@Keksike
Copy link
Author

Keksike commented Apr 27, 2018

Excellent points! You are absolutely right that there should be a way to include multiple paths, and I don't think there is a good way to do it with the command line switches.

My gut feeling is that an optional config would be the way to go if you decide to add this feature in.

Anyways, thanks for responding and I hope my PR raises some discussion!

@Keksike
Copy link
Author

Keksike commented May 3, 2018

https://github.com/txchen/light-server has a similar kind of implementation as the one I did (--proxypath <proxypath> e.g. --proxypath '/api/'). I found it perfect for my needs.

It also has a optional config file, which might be a good way to implement the configuration (e.g. for various proxypaths) which was mentioned above.

@github-actions
Copy link

This pull request has been inactive for 360 days

@github-actions github-actions bot added the stale label Aug 25, 2021
@one-github
Copy link

one-github commented Sep 29, 2021

This was almost working, except res.emit('next'); was missing:

diff --git a/node_modules/http-server/bin/http-server b/node_modules/http-server/bin/http-server
index fefa681..0454642 100755
--- a/node_modules/http-server/bin/http-server
+++ b/node_modules/http-server/bin/http-server
@@ -127,6 +127,7 @@ function listen(port) {
     ext: argv.e || argv.ext,
     logFn: logger.request,
     proxy: proxy,
+    proxyPath: argv.R,
     showDotfiles: argv.dotfiles,
     mimetypes: argv.mimetypes,
     username: argv.username || process.env.NODE_HTTP_SERVER_USERNAME,
diff --git a/node_modules/http-server/lib/http-server.js b/node_modules/http-server/lib/http-server.js
index 996db6f..926a56c 100644
--- a/node_modules/http-server/lib/http-server.js
+++ b/node_modules/http-server/lib/http-server.js
@@ -143,6 +143,13 @@ function HttpServer(options) {
   if (typeof options.proxy === 'string') {
     var proxy = httpProxy.createProxyServer({});
     before.push(function (req, res) {
+       var shouldNotProxy =
+         options.proxyPath && req.url.substring(0, options.proxyPath.length) !== options.proxyPath;
+
+       if (shouldNotProxy) {
+         res.emit('next');
+         return;
+       }
       proxy.web(req, res, {
         target: options.proxy,
         changeOrigin: true

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.

3 participants