http.request(url.parse(href)) fails

[jonathanong]

i'm not totally sure what's going on, but if i use this parser, url.parse(href) is no longer a valid object to pass to node's http.request(). just from looking at the difference, it looks like you don't set the path and href property on the parsed object, which http.request() might need.

i just stopped using this module - too lazy to debug right now.

[petkaantonov]

I can fix this on saturday. Those properties simply need to be changed from lazy to eager like they are in node url.

Of course fix should be in http_agent not to rely on internal implementation details of the URL module and use the public api only but that's not going to happen.Nevermind that, you are supposed to pass options object, not url object to http.request. The options object shape accidentally shares a few properties with URL objects which made it work before.

[jonathanong]

yeah it's not "supposed" to be the parsed object, but it's designed to be compatible with them like with hostname http://nodejs.org/api/http.html#http_http_request_options_callback. what i do is parse a url then add options to it. i haven't looked at node's code so i'm not sure what's need to be done. i get how a lot of stuff are getters for performance here, prefer not to take a performance hit just for this.

[petkaantonov]

Node's code is using Object.keys to copy the URL's properties to the plain options object. But the properties in this implementation are actually getters on the prototype so they are not picked up by that.

https://github.com/joyent/node/blob/master/lib/_http_agent.js#L291-L294

https://github.com/joyent/node/blob/master/lib/util.js#L584-L594

[jonathanong]

oh so why not just make the getters enumerable? would that work?

[petkaantonov]

Well no, because .keys only looks at own properties.

[jonathanong]

errr right. forgot this was a prototype. i should look at code more before speaking :P

[petkaantonov]

So turns out it's not actually that much performance lost if they were normal eager properties (400k -> 230k parses per second on the benchmark). However by now refactoring is probably going to mean full rewrite level of effort (damn me for not doing it on that saturday I promised).

[jonathanong]

@petkaantonov i would wait to see what the io.js tc decides on the issue above. might save you some work :) i like the current implementation personally

[jonathanong]

should be fixed in iojs :D