Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

url + http/https: weird api between these module #8722

Closed
yorkie opened this issue Nov 13, 2014 · 9 comments
Closed

url + http/https: weird api between these module #8722

yorkie opened this issue Nov 13, 2014 · 9 comments

Comments

@yorkie
Copy link

yorkie commented Nov 13, 2014

url.format will return an object owns pathname rather than path, however http.request and https.request do accept path rather than pathname.

sometimes, we would seem to write our program:

var urlobj = url.format('http://github.com/yorkie');
http.request(urlobj, function (res) {
  // actually we get the response from http://github.com
});

path is missing, yeah actually we can add urlobj.path = urlobj.pathname for now, it does work as well, but it's a weird behavior in api level imo.

I'm guessing we should unify these two/three functions definitely, of course backward compatible with url module.

@yorkie
Copy link
Author

yorkie commented Nov 13, 2014

sorry, i mean url.format doesn't accept path, the case in real world is https://github.com/yorkie/node-httpmocker/blob/master/index.js#L39.

@yorkie
Copy link
Author

yorkie commented Nov 20, 2014

any thoughts from core team?

@indutny
Copy link
Member

indutny commented Nov 20, 2014

What node.js version are we talking about?

@indutny
Copy link
Member

indutny commented Nov 20, 2014

Also, could you please paste a complete example of this thing, i.e. the code that works with http module and doesn't with https.

@yorkie
Copy link
Author

yorkie commented Nov 20, 2014

From the latest stable docs:

the complete example should be written at https://github.com/yorkie/node-httpmocker/blob/master/index.js#L39, now i copy/paste it here:

function mockRequest(options, callback) {
  var requesturl = url.format(options);
  // ...
}

The function mockRequest will be called like http.request or https.request because this function is mocking these 2 official APIs.

Hence that the progress is: calling mockRequest({host: 'github.com', path: '/induty'}) would be equal to url.format({host: 'github.com', path: '/induty'}), and path would be ignored by url.format at that moment. To let example work, we have to add a line: options.pathname = options.path.

To reproduce this on v0.10.33:

> url.format({protocol:'http',host:'github.com',path:'/induty'})
'http://github.com'
> url.format({protocol:'http',host:'github.com',pathname:'/induty'})
'http://github.com/induty'

@yorkie
Copy link
Author

yorkie commented Nov 20, 2014

and this actually is not related http or https, but never mind, it's not a bug just wanna get thoughts from core team 🌸

@indutny
Copy link
Member

indutny commented Nov 20, 2014

Oh, I see. It doesn't look like there could be a reason why it should not support path as well as pathname. Mind submitting a PR with test? ;)

@yorkie
Copy link
Author

yorkie commented Nov 20, 2014

bingo, then would do that later 🍒

@yorkie yorkie mentioned this issue Nov 20, 2014
4 tasks
chrisdickinson pushed a commit that referenced this issue Dec 2, 2014
this adds support for a "path" field that overrides
"query", "search", and "pathname" if given.

Fixes: #8722
PR-URL: #8755
Reviewed-by: Chris Dickinson <[email protected]>
@chrisdickinson
Copy link

Fixed in d312b6d.

chrisdickinson pushed a commit to nodejs/node that referenced this issue Dec 2, 2014
this adds support for a "path" field that overrides
"query", "search", and "pathname" if given.

Fixes: nodejs/node-v0.x-archive#8722
PR-URL: nodejs/node-v0.x-archive#8755
Reviewed-by: Chris Dickinson <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants