Make constructor throw on invalid ports.

[konstantinblaesi]

This makes the behaviour consistent with port().

Fixes #344

[rodneyrehm]

this change should be covered by a test for the constructor (test/test.js, line 132)

[konstantinblaesi]

I added a negative and a positive test, one for each case. Is that enough?

[rodneyrehm]

I don't think we need a positive test case, there are plenty of those :)

[konstantinblaesi]

Ok, I've removed the positive testcase, improved the port validation code and 3 negative tests in total.

[rodneyrehm]

The validation should probably be done in URI.parseHost()? maybe URI#hostname()'s validation can be moved there, too?

[konstantinblaesi]

I've created and called the method this way because I wanted to put some validation there that isn't just about a specific part of the url but about a combination such as protocol, hostname, path.

[rodneyrehm]

I don't think we need a positive test case, there are plenty of those :)

[rodneyrehm]

should this be called ensureValidHostname()?

[konstantinblaesi]

I put it here because I just wanted to prevent "http:///" being a valid input for the constructor. Since I am not only checking the hostname, but also protocol and path I extracted it into it's own function.

[rodneyrehm]

this is true of any protocol, even the empty protocol ://hostname/path

[konstantinblaesi]

I've updated the code, because apparently my single negation was wrong anyways.

[konstantinblaesi]

Looks like rewording/editing/amending commits isn't a good idea for implementing requested changes :( It's probably better to add new commits and do the squashing/merging when it's ready to merge?

[rodneyrehm]

this is not limited to HTTP/S, but also applies to ssh, ftp, and many more. Maybe we should create URI.hostProtocols analoguous to URI.defaultPorts? The name of this function should be similar to the property, so URI.ensureHostProtocol() or something like that? either way, basic is not a thing in URLs, so the term is really irritating.

[konstantinblaesi]

Are you suggesting to reject all URLs of the form protocol:/// no matter what the protocol is? Would you use URI.hostProtocols to store different regexes for the validation of the individual protocols?

[rodneyrehm]

sorry for the delay… no, I'm suggesting putting the protocols for which we require a hostname into a map on URI, like it's done for URI.defaultPorts. That would allow anyone to easily add the protocols they want validated the same way

[konstantinblaesi]

@rodneyrehm What do you think, is this ready to merge?

[rodneyrehm]

could be simplified to

URI.ensureValidPort = function(v) {
  if (!v) {
    return;
  }

  var port = Number(v);
  if (Number.isInteger(port) && (port > 0) && (port < 65536)) {
    return;
  }

  throw new TypeError('Port "' + v + '" is not a valid port');
};

[konstantinblaesi]

done

[rodneyrehm]

thanks for your effort! released as v1.18.11 :)

[rodneyrehm]

Hey @konstantinblaesi,

since the introduction of hostname verification in v1.18.11 we've seen a few compatibility problems: #352, #354, #355.

I'm considering the rollback of your change - unless you have a better idea?

[rodneyrehm]

As the hostname validation caused some kerfuffle I've decided to make this feature opt-in, thereby returning to the previous default behavior of v1.18.10. As of v1.19.0 you can activate hostname validation globally by setting URI.preventInvalidHostname = true, or on an instance by calling .preventInvalidHostname(true).

I'm sorry for the confusion, inconvenience caused by the original change and the delayed rectification.