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

Properly parse urls without "//" after the protocol #22

Merged
merged 2 commits into from
Mar 27, 2016
Merged

Properly parse urls without "//" after the protocol #22

merged 2 commits into from
Mar 27, 2016

Conversation

iamtheib
Copy link
Contributor

Fixes issue #8

let url = parse('mailto:some@one.co'); // starting out with no slashes
url.set('protocol', 'https');
url.href === 'https://some@one.co'; // true

url = parse('https://github.com'); // starting out with slashes
url.set('protocol', 'sip', true);
url.href === 'sip:github.com'; // true

url.protocol = 'http' // this will not behave as intended
url.toString() === 'http://github.com'; // false

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0838acd on iamtheib:master into 7d03c18 on unshiftio:master.

var url = this;

if ('query' === part) {
if (fnOrNoSlashes && 'function' !== typeof fnOrNoSlashes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not add this, it will throw an error anyway if the given argument is not function.
It will save few bytes on the browser.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

@lpinca
Copy link
Member

lpinca commented Mar 27, 2016

Left some comments but LGTM. It would be nice to have another pair of eyes here anyway.
Thank you.

@@ -29,6 +29,19 @@ var instructions = [
];

/**
* Extract protocol information from a URL
* Correctly extracts protocols with/without double slash ("//");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you JSDoc comment this function? So it documented what it returns, and what the function expects to receive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is meant to be an internal/private function not exposed in the API. I can still use JSDoc for it I guess

@3rd-Eden
Copy link
Member

Minor comments, but LGTM otherwise

@iamtheib
Copy link
Contributor Author

Made the suggested changes. @lpinca I left the for loop in the test

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c707f0e on iamtheib:master into 7d03c18 on unshiftio:master.

* Extract protocol information from a URL with/without double slash ("//")
*
* @param {String} address URL we want to extract from.
* @return {ProtocolExtract} Extracted information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was ok with just Object but thanks for the effort.

@lpinca
Copy link
Member

lpinca commented Mar 27, 2016

👍

@3rd-Eden 3rd-Eden merged commit 3114083 into unshiftio:master Mar 27, 2016
@3rd-Eden
Copy link
Member

Ill release a new minor in a bit. Thanks for your contribution

@iamtheib
Copy link
Contributor Author

No problem. Happy to help.

@lpinca
Copy link
Member

lpinca commented Mar 27, 2016

This test fails on IE, we need to fix it.

@iamtheib
Copy link
Contributor Author

Yes. Just seeing it. It has to do with the expected error message.

@lpinca
Copy link
Member

lpinca commented Mar 27, 2016

On my way.

@iamtheib
Copy link
Contributor Author

End of this line can probably be changed to throws(/function/i). Or just assume it throws(), if that's possible

@lpinca
Copy link
Member

lpinca commented Mar 27, 2016

02a9296

@3rd-Eden
Copy link
Member

Published as 1.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants