Port is not validated the same when constructing URI as when using uri.port() method

[fredrikekelund]

// The following works (when I think it shouldn't)
var uri = URI('http://example.com:robots.txt');

// Even this works
console.log(uri.port()); // Will output 'robots.txt'

// This does not work however...
var uri = URI('http://example.com');

// ...since this will (correctly) throw an error
uri.port('robots.txt');

Could we look into adding the same validation of the URI port when constructing URI instances from strings as we have when calling the uri.port method?

[rodneyrehm]

nice catch!

I don't think the constructor currently throws any errors so this would be the first. It would be possible to consider anything that not a valid port as part of the path in order to maintain the liberal parsing behavior. That would allow the following normalizations:

http://example.com:robots.txt
-> http://example.com/robots.txt

http://example.com:foo/robots.txt
-> http://example.com/foo/robots.txt

[fredrikekelund]

Ah I see, well it seems reasonable not to introduce unique behavior just for this scenario if the constructor doesn't throw for anything else.

To me the behavior you're proposing makes sense! I do think most scenarios with a faulty port in the constructor URL will actually be wanting to make that port part of the pathname. It's still making a pretty big assumption, but if that's what people come to URI.js for, then I say 👍

[rodneyrehm]

Are you up for a PR to fix this?

[fredrikekelund]

@rodneyrehm I am not familiar with the code base, but I can have a look and see what I can do! Might be a little while before I have time to submit it, though

[konstantinblaesi]

Are you saying that

const urijs = require('urijs');
urijs('http://host:port');
urijs('http:///');

should not throw? Or would you agree that the validation logic that is part of the setters should be extracted and called in the constructor/factory as well and if any error is found it should be thrown so that users can take appropriate action without ending up with broken urls? Or would you defer the validation logic to methods like toString(), validate(), etc. ?

[konstantinblaesi]

@rodneyrehm why would you reject "port" as port in port(), but allow it in the constructor/factory?

[rodneyrehm]

why would you reject "port" as port in port(), but allow it in the constructor/factory?

»I don't think the constructor currently throws any errors so this would be the first.« is the only "reason". The parser is pretty permissive with everything else. But I do agree that it makes little sense. I'm fine with the parser throwing errors on invalid URLs that simply make no sense.

[konstantinblaesi]

@rodneyrehm Thanks for clarifying 👍 Here's my first attempt at fixing this. Something similar happens when the hostname is missing, such as

const urijs = require("urijs");
let url = urijs("http:///"); // does not throw
url.hostame(""); // does throw

I might create a separate issue/pr or append changes needed. What do you think?

[rodneyrehm]

»I don't think the constructor currently throws any errors so this would be the first.« is the only "reason".

Apparently I was wrong, as new URI(null) does throw.

I might create a separate issue/pr or append changes needed. What do you think?

go ahead and append them.