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

Remove url fragments from host name #41

Merged
merged 6 commits into from
May 21, 2014

Conversation

jhnns
Copy link
Contributor

@jhnns jhnns commented May 20, 2014

When working with tld.js I thought it might be useful if tld.js would also work with host names containing url fragments:

tld.getDomain("http://fr.google.com/some/path")
// should return 'google.com' imho
tld.tldExists('https://user:password@example.co.uk:8080/some/path?and&query#hash');
// should return `true`
tld.getSubdomain('https://moar.foo.google.co.uk');
// should return `moar.foo`

The pull-request contains tests for this feature and updates the README accordingly.

@thom4parisot
Copy link
Owner

Hey thanks!

It addresses an issue raised by #38. I wonder why you do not use directly the require('url').parse native Node's function?

@jhnns
Copy link
Contributor Author

jhnns commented May 21, 2014

Yeah, I could also use require('url').parse. I thought that we don't need all the other features of the url-module and thus only need a regex which captures the relevant parts. I think the regex is safe to use, I used it in other projects too.

But if you feel more comfortable with the url-module, I can change that 😄

@jhnns
Copy link
Contributor Author

jhnns commented May 21, 2014

I wonder if that extra logic should be the responsibility of the consumer or the library.

I thought about this question too. Usually I'm a big fan of small libs which do one thing well. But I also like clear and simple APIs and imho it's ok to provide a little bit extra logic if I can save some lines in the application. It's definitely not an uncommon use-case to extract the domain portion of an url.

But I guess this is a matter of taste. 😉

BTW: Travis CI seems to fail because of node 0.8 incompatible dependencies.

@thom4parisot
Copy link
Owner

@jhnns okay got it :-) I wanted to hear more about that before merging.

Rebase from master, I have removed Node 0.8 compatibility and updated modules anyway.

@jhnns
Copy link
Contributor Author

jhnns commented May 21, 2014

Ok, ready to merge

thom4parisot pushed a commit that referenced this pull request May 21, 2014
Remove url fragments from host name
@thom4parisot thom4parisot merged commit 0e29550 into thom4parisot:master May 21, 2014
@jhnns
Copy link
Contributor Author

jhnns commented May 21, 2014

👍

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.

None yet

2 participants