Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verifying url.format parameters #1764

Closed
goloroden opened this issue May 22, 2015 · 2 comments
Closed

Verifying url.format parameters #1764

goloroden opened this issue May 22, 2015 · 2 comments
Labels
url Issues and PRs related to the legacy built-in url module.

Comments

@goloroden
Copy link

I have been using the code below:

request.post(url.format({
  protocol: 'http',
  host: 'localhost',
  port: port,
  pathname: '/my-route'
}), {
  body: { value: 'foobar' },
  json: true
}, function (err, res, body) {
  if (err) {
    return console.error(err);
  }
  console.log('Sent data.');
});

All I get is an EPIPE error. Finally, after two hours (!), I figured out what the problem was: It should be

url.format({
  protocol: 'http',
  hostname: 'localhost',
  port: port,
  pathname: '/my-route'
})

and not:

url.format({
  protocol: 'http',
  host: 'localhost',
  port: port,
  pathname: '/my-route'
})

So it's host vs hostname, and about ignoring the port when host is given. This error is quite annoying for several reasons:

  • In fairly complex applications, you first look for the error everywhere else (where more logic is involved).
  • host vs hostname is too easy to mix up.
  • There is no error message telling you that it doesn't make sense to provide a port if you use host instead of hostname.

So, to cut a long story short: Would verifying this be helpful?

What I'm thinking of is just a small test that checks whether you provide host and port. If so, it should tell you that port is ignored and you should either remove it, or use hostname instead.

What do you think?

@petkaantonov
Copy link
Contributor

It's a bit more involved than that because url objects have host, hostname and port and would always cause that warning.

Also it's easier and more readable to use:

request.post(`http://localhost:${port}/my-route`);

@silverwind silverwind added the url Issues and PRs related to the legacy built-in url module. label May 22, 2015
@jasnell
Copy link
Member

jasnell commented Mar 9, 2016

Closing this given the lack of any activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

No branches or pull requests

4 participants