Minor change to allow proxy.pac users #302

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
@choonkeat

Allowing HTTPServer to be slightly lenient and allow working with "proxy.pac" setups like http://stackoverflow.com/questions/684558/setting-up-wildcard-domains-on-local-host-os-x-10-5/684998

@tj

View changes

lib/http.js
@@ -176,23 +176,24 @@ Server.prototype.handle = function(req, res, out) {
}
try {
- var pathname = parse(req.url).pathname;
- if (undefined == pathname) pathname = '/';
+ var urlobj = url.parse(req.url);

This comment has been minimized.

Show comment Hide comment
@tj

tj Jun 21, 2011

Member

definitely don't like the urlobj

@tj

tj Jun 21, 2011

Member

definitely don't like the urlobj

This comment has been minimized.

Show comment Hide comment
@choonkeat

choonkeat Jun 22, 2011

the name can change to something else if that's what you mean? it was taken from the doc url.format(urlObj)

@choonkeat

choonkeat Jun 22, 2011

the name can change to something else if that's what you mean? it was taken from the doc url.format(urlObj)

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jun 21, 2011

Member

it's not clear what you are even changing amongst the stylistic changes, what does the proxy.pac thing require?

Member

tj commented Jun 21, 2011

it's not clear what you are even changing amongst the stylistic changes, what does the proxy.pac thing require?

@choonkeat

This comment has been minimized.

Show comment Hide comment
@choonkeat

choonkeat Jun 22, 2011

Apologies for not being clearer.

The change allows for req.url either be pathnames GET /path HTTP/1.0 or full urls GET http://host.local/path HTTP/1.0

Apologies for not being clearer.

The change allows for req.url either be pathnames GET /path HTTP/1.0 or full urls GET http://host.local/path HTTP/1.0

@choonkeat

View changes

lib/http.js
@@ -176,23 +176,24 @@ Server.prototype.handle = function(req, res, out) {
}
try {
- var pathname = parse(req.url).pathname;

This comment has been minimized.

Show comment Hide comment
@choonkeat

choonkeat Jun 22, 2011

since we're already parsing req.url, we might as well manipulate the parsed object and update req.url properly in the end, via req.url = url.format(urlobj);

@choonkeat

choonkeat Jun 22, 2011

since we're already parsing req.url, we might as well manipulate the parsed object and update req.url properly in the end, via req.url = url.format(urlobj);

This comment has been minimized.

Show comment Hide comment
@TooTallNate

TooTallNate Jun 22, 2011

Contributor

-1 for overwriting req.url with an Object. That would be very unexpected.

@TooTallNate

TooTallNate Jun 22, 2011

Contributor

-1 for overwriting req.url with an Object. That would be very unexpected.

This comment has been minimized.

Show comment Hide comment
@choonkeat

choonkeat Jun 22, 2011

no, req.url doesn't become an object, it is still a string, re-composed by the url.format function (reverse of url.parse)

@choonkeat

choonkeat Jun 22, 2011

no, req.url doesn't become an object, it is still a string, re-composed by the url.format function (reverse of url.parse)

This comment has been minimized.

Show comment Hide comment
@TooTallNate

TooTallNate Jun 22, 2011

Contributor

Oh ya, my bad. Read that wrong

Sent from my iPhone

On Jun 21, 2011, at 19:55, choonkeatreply@reply.github.com wrote:

@@ -176,23 +176,24 @@ Server.prototype.handle = function(req, res, out) {
}

try {
  •  var pathname = parse(req.url).pathname;
    

no, req.url doesn't become an object, it is still a string, re-composed by the url.format function (reverse of url.parse)

Reply to this email directly or view it on GitHub:
https://github.com/senchalabs/connect/pull/302/files#r49605

@TooTallNate

TooTallNate Jun 22, 2011

Contributor

Oh ya, my bad. Read that wrong

Sent from my iPhone

On Jun 21, 2011, at 19:55, choonkeatreply@reply.github.com wrote:

@@ -176,23 +176,24 @@ Server.prototype.handle = function(req, res, out) {
}

try {
  •  var pathname = parse(req.url).pathname;
    

no, req.url doesn't become an object, it is still a string, re-composed by the url.format function (reverse of url.parse)

Reply to this email directly or view it on GitHub:
https://github.com/senchalabs/connect/pull/302/files#r49605

This comment has been minimized.

Show comment Hide comment
@tj

tj Jun 22, 2011

Member

I don't really see the point of that, primarily because it can change per middleware. This is the reason each middleware using the url parses it again, it's a reasonably small cost though

@tj

tj Jun 22, 2011

Member

I don't really see the point of that, primarily because it can change per middleware. This is the reason each middleware using the url parses it again, it's a reasonably small cost though

@choonkeat

View changes

lib/http.js
if (nextChar && '/' != nextChar && '.' != nextChar) return next(err);
// Call the layer handler
// Trim off the part of the url that matches the route
removed = layer.route;
- req.url = req.url.substr(removed.length);
+ urlobj = {pathname: urlobj.pathname.substr(removed.length), search: urlobj.search};

This comment has been minimized.

Show comment Hide comment
@choonkeat

choonkeat Jun 22, 2011

This line was supposed to drop the extraneous url properties (e.g. protocol, hostname, port, etc) and return things to "normalcy" of working with /path only.

Turns out it is unnecessary to remove those information (not worse off than now). Info can, and should, be kept intact for the down-chain, with this instead:

urlobj.pathname = urlobj.pathname.substr(removed.length);
@choonkeat

choonkeat Jun 22, 2011

This line was supposed to drop the extraneous url properties (e.g. protocol, hostname, port, etc) and return things to "normalcy" of working with /path only.

Turns out it is unnecessary to remove those information (not worse off than now). Info can, and should, be kept intact for the down-chain, with this instead:

urlobj.pathname = urlobj.pathname.substr(removed.length);
@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jun 22, 2011

Member

ah, I see the intention now, I'm sure we can implement that with a minimal patch. middleware shouldn't assume it's a path onl y anyway so parsing is always necessary

Member

tj commented Jun 22, 2011

ah, I see the intention now, I'm sure we can implement that with a minimal patch. middleware shouldn't assume it's a path onl y anyway so parsing is always necessary

@choonkeat

This comment has been minimized.

Show comment Hide comment
@choonkeat

choonkeat Jun 22, 2011

yay!

yay!

choonkeat added some commits Jun 22, 2011

Was supposed to drop the extraneous url properties (e.g. protocol, ho…
…stname, port, etc) and return things to "normalcy" of working with ``/path`` only.

Turns out it is unnecessary to remove those information (not worse off than now). Info can, and should, be kept intact for the down-chain
@choonkeat

This comment has been minimized.

Show comment Hide comment
@choonkeat

choonkeat Jul 5, 2011

renamed to a minimal u to follow recent commit convention

renamed to a minimal u to follow recent commit convention

@jonathanong

This comment has been minimized.

Show comment Hide comment
@jonathanong

jonathanong Oct 17, 2013

Contributor

@dougwilson related to #920? maybe you want to try fixing this too

Contributor

jonathanong commented Oct 17, 2013

@dougwilson related to #920? maybe you want to try fixing this too

@dougwilson

This comment has been minimized.

Show comment Hide comment
@dougwilson

dougwilson Oct 17, 2013

Contributor

Yea, as far as I can tell, #920 fixes this.

Contributor

dougwilson commented Oct 17, 2013

Yea, as far as I can tell, #920 fixes this.

@jonathanong

This comment has been minimized.

Show comment Hide comment
@jonathanong

jonathanong Oct 17, 2013

Contributor

too bad there are no tests. closing

Contributor

jonathanong commented Oct 17, 2013

too bad there are no tests. closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment