Allow proxy to maintain the original target path

[EndangeredMassa]

This completes the work started in: #645

• Fixed tests
• Added a test for the path behavior

[jcrugzz]

@EndangeredMassa One thing I would do though is also support forward requests. Follow the convention we use of options[forward || 'target']. Otherwise this is great, thanks for the effort!

[EndangeredMassa]

I updated it, but I'm not really clear on how forward is supposed to be used here. Let me know if I need to tweak it.

[jcrugzz]

@EndangeredMassa its passed in as a variable so it does an undefined check relying on the || operator to default to 'target' if its not a forward request. See here.

And this looks great, thanks!

[jcrugzz]

Im realizing that this shouldnt necessarily be default behavior. This should be configured in some way. @EndangeredMassa naming suggestions? Seems like a boolean option is the best way to deal with this.

[EndangeredMassa]

Hrm. I expect this behavior to be default. In my mind, adding a url that includes a path in the target means that I want to use that full url, not just the host, port, and protocol.

[jcrugzz]

@EndangeredMassa Yea that's understandable. Im guessing our case at nodejitsu is the special case as it is more specific to how couchdb interprets the request. We currently proxy to couchdb instances with smart-private-npm by setting the host to registry.nodejitsu.com for example and the URL is https://myCouchdb.com/registry. This change currently breaks the way we have been proxying. Ill think of a good option for this I suppose as that seems like it will be what is necessary.

[indexzero]

http://nodejsreactions.tumblr.com/post/66103133377/pull-req-that-fixes-a-bug-includes-docs-and-tests-and

[manast]

It should not be default unless a major version update, since this breaks backwards compatibility... we ended with such a problem in redbird, that already appends the target path if necessary: https://github.com/OptimalBits/redbird

[indexzero]

@manast it does not break backwards compatibility. It introduces a new feature that allows for the path to be part of the target when creating HttpProxy instances.

[manast]

@indexzero in my opinion that is not really the case, because even if not supported before, if you happen to have a path in the target, the new behaviour would break things. It is a bit in the grey zone, but I think the safest bet would have been to have it disabled by default, users wanting to use the new feature would have been forced to enable it manually. For instance, before this feature existed, many people created its own third party code for adding it, this code usually left the target with the complete path since it was not necessary to parse it just to remove it.
Anyway, it is too late now for anything so pointless discussion other than flagging that since http-proxy is so critical in many deployments, maintaining backwards compatibility should be taking very seriously.

[indexzero]

We do take backwards compatibility very seriously, and I will say it again: this is backwards compatible. Objects that node-http-proxy accepts should have only the properties that node-http-proxy expects and should be expected to be mutated by node-http-proxy. If a user was passing in superfluous properties that are now meaningful then they were using the library incorrectly to begin with.

If you would like to update the documentation about this point I would greatly appreciate it.