Strip the pathPrefix from basic proxy requests

[stramel]

Addresses issues that were brought up in #393

• Adds secure: false back in
• Drops ignorePath: true which strips the entire path of the request. Replaced it with a default proxyReq event listener that just strips the pathPrefix off the path.

proxy: {
  "/storybook": "http://localhost:3000"
}

This will result in the URL going from http://localhost:8080/storybook/static/js/bundle.js -> http://localhost:3000/static/js/bundle.js which, I believe, was the intention of that config entry.

For the advanced configs, I'm going to leave it as whatever you configure is what you get.

@FredKSchott
/cc @fcamblor

[FredKSchott]

Yup, that makes sense and matches what the http-proxy docs say about it

[germanftorres]

Hi,

@stramel Great to see how this feature is improving thanks to your recent PRs.

Anyway, I'm not 100% sure that stripping the requestPath is enough. In my testing there are some inconsitencies you may have a look at.

In case the js app makes this call fetch('/api/status') this is what happens in different config scenarios:

  "proxy": {

    "/api": "http://localhost:5000",  
// -> prefix removed, target localhost:5000 receives /status

    "/api": "http://localhost:5000/some/other/path",  
// -> request prefix removed but target path is not considered at all, maybe a bug
// -> target localhost:5000 still receives   /status,

    "/api": {
      "target": "http://localhost:5000" 
    } ,
// -> no prefix stripping takes place and target receives full url   /api/status

    "/api": {
      "target": "http://localhost:5000/api" 
    } 
// -> no prefix stripping takes place and target path is considered
// -> target receives this  /api/api/status

  }

I would check these individual cases if the intention is, for the most basic proxy configuration, to strip the prefix path from the request and add the path from the target. Would also weigh the upgrade case from the developers perspective when he want's to change a string configuration to a full options config. Ideally he would be able to migrate a simple config from "string" to { "target": "string" } without breaking anything.

Thanks!!

Germán

[stramel]

@germanftorres

@stramel Great to see how this feature is improving thanks to your recent PRs.

Thanks! 😄

As of now, I have been working to make the basic setup function similarly to how the proxy in the scripts section worked.

So going through your examples

fetch('/api/status')

"proxy": {

    "/api": "http://localhost:5000",  
	// -> prefix removed, target localhost:5000 receives /status
	// EXPECTED: /api/status -> http://localhost:5000/status

    "/api": "http://localhost:5000/some/other/path",  
	// -> request prefix removed but target path is not considered at all, maybe a bug
	// -> target localhost:5000 still receives   /status,
	// EXPECTED: /api/status -> http://localhost:5000/some/other/path/status

    "/api": {
      "target": "http://localhost:5000" 
    },
	// -> no prefix stripping takes place and target receives full url /api/status
	// EXPECTED: /api/status -> http://localhost:5000/api/status

    "/api": {
      "target": "http://localhost:5000/api" 
    } 
	// -> no prefix stripping takes place and target path is considered
	// -> target receives this  /api/api/status
	// EXPECTED: /api/status -> http://localhost:5000/api/api/status

  }

Once you eject to an object config, we are deferring everything to you. It's pretty simple to do the proxy as you can see in this PR. Just add:

"proxy": {
  "/api": {
    "target": "http://localhost:5000",
	"on": {
	  "proxyReq": (proxyReq, req) => proxyReq.path = req.url.replace('/api')
	}
  },
}

I suppose we probably could add that rewrite to all requests by default and require users to prepend the /api if they don't want us to remove it?

It looks like the second example isn't working properly, I can look into that.

Ideally he would be able to migrate a simple config from "string" to { "target": "string" } without breaking anything.

I will have to defer to @FredKSchott on this.

[FredKSchott]

Yup, thanks for the summary @stramel, that all looks right.

I'm +1 for not doing too much magic in the object use-case, I feel like it will confuse as many people as it would help (harder to trust the http-proxy README docs).