-
Notifications
You must be signed in to change notification settings - Fork 476
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
Feature/to uri #374
base: gh-pages
Are you sure you want to change the base?
Feature/to uri #374
Conversation
Hey there, thanks for chipping in! I'll comment on the code itself as well, but generally you should:
|
@rodneyrehm Makes sense. Do you want to go ahead and close this one and Ill make the changes and re-submit this afternoon? |
@@ -487,6 +487,36 @@ | |||
|
|||
URI.encodeReserved = generateAccessor('reserved', 'encode'); | |||
|
|||
URI.toUri = function(string, defaults) { | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were doing doc-blocks, they'd come before the function, not within.
switch (true) { | ||
case defaults.scheme === 'http': | ||
case defaults.scheme === 'https': | ||
defaults.scheme = defaults.scheme+':'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're mutating a an object that was passed into the function, this can have unexpected consequences. consider a simplified version of your code:
function doSomething(defaults) {
switch (true) {
case defaults.scheme === 'http':
case defaults.scheme === 'https':
defaults.scheme = defaults.scheme+':';
break;
default:
defaults.scheme = '';
}
console.log(defaults)
}
var myDefaults = { scheme: 'http' };
doSomething(myDefaults);
doSomething(myDefaults);
and the output:
{scheme: "http:"}
{scheme: ""}
// be parsed as a path | ||
string = defaults.scheme+'//'+string; | ||
} | ||
return URI.build(URI.parse(string)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the benefit of running a parse+serialization at this point?
defaults = defaults || {}; | ||
// if the uri doesn't have a scheme, add one | ||
// now so it doesn't get parsed as a path | ||
if (!string.match(/^(https|http)?:\/\//i)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also match URLs of the form ftp://…
which would then produce http://ftp://…
.
You'd be looking for the index of "://"
and taking the the substring that comes before. You'd add the default.scheme
only if "://"
is not part of the string, or the substring before that is the empty string.
https://github.com/medialize/URI.js/blob/gh-pages/src/URI.js#L218 is a ready-to-use expression for validating a scheme, if should that be necessary.
you can edit the existing PR, or you can close this PR, whatever you prefer ;) |
When initializing with a string that is missing the protocol, like
URI('somesite.com');
, the string is parsed as a path and not a URI.Because of this,
domain(), host(), and port()
do not function as expected and return "";See https://jsfiddle.net/Ln9c0jpy/1/
Related issues:
#260
#232
#187
This change adds a
toUri
method that allows the user to do this:let uri = new URI(URI.toUri('somesite.com', {scheme: 'http'}));
after which,
uri.domain()
will return the expectedsomesite.com