Add public_suffix method to allow users to access the PublicSuffix::Domain object easily. #510
Conversation
PublicSuffix::Domain object easily. This also provides a consistent interface for Addressable::URI to interact with.
a bit hesitant about this, sure this object looks simple enough now, but what if it changes down the road? There could be unexpected breaking changes for users of addressable then |
|
Addressable already relies on it, so it would need to be updated for breaking changes as well. It also seems pretty unlikely to change since it only handles parsing domains. I've been working with a lot of apps that allow custom domains & subdomains and it would be really helpful for Addressable to support parsing those out. I was surprised that wasn't a feature considering it was already using PublicSuffix. I can merge the |
Add subdomain and subdomains methods
|
Decided to merge the subdomain methods into this PR to show the complete feature. |
Yes, but we own the API, and I think it is a difference in interacting with PublicSuffix to get a basic value (e.g. String) then returning a full PublicSuffix object.
Yeah, |
|
What do you think works best? Should we keep the |
|
I'm not sold on these additions. I'm feel there's ambiguity around the terms "subdomain" and "subdomains"... this looks a bit different from what publicsuffix-ruby does? irb(main):006:0> PublicSuffix.parse("www.google.com", ignore_private: true).subdomain
=> "www.google.com" |
That was on purpose. I wanted a way to access the subdomains (without the primary domain). The |
| # @example | ||
| # Addressable::URI.parse("http://www.test.example.co.uk").domain # => "www" | ||
| def subdomain | ||
| subdomains.first |
There was a problem hiding this comment.
Still think there's ambiguity around what's a subdomain and what a method like this is expected to return.
www is a subdomain of test.example.co.uk
test is a subdomain of example.co.uk
example is a subdomain of co.uk
This method would return nil when parsing http://example.co.uk
There was a problem hiding this comment.
I was modeling this off of Rails which returns the first subdomain, but let me know what you'd like this to do. 👍
There was a problem hiding this comment.
I'm not familiar with that in Rails, can you link the source for method? When is it typically used?
I'm not sure what we want here, if the method should exist?
There was a problem hiding this comment.
Multi-tenant applications or anything that uses subdomains (think Shopify) use this. I've always been surprised there's no way to get the subdomain(s) from URIs in Ruby. Rails will do it for the current request, but that's it and it's not Public Suffix aware. You can only specify a single TLD for your entire Rails app, so you need to reach for something else like addressable + public_suffix.
https://api.rubyonrails.org/classes/ActionDispatch/Http/URL.html#method-i-subdomain
There was a problem hiding this comment.
Yeah... if we should add this method, is it not better if it matches the subdomain on ActionDispatch? From the URL above
Returns all the subdomains as a string, so "dev.www" would be returned for “dev.www.rubyonrails.org”.
For http://www.test.example.co.uk that would be www.test, not just www?
There was a problem hiding this comment.
What about not adding the #subdomain method? Instead only #subdomains. It is easy for the user to do subdomains.first or subdomains.first(2) etc. depending on what they want.
| # Returns the first subdomain for this host. | ||
| # | ||
| # @example | ||
| # Addressable::URI.parse("http://www.test.example.co.uk").domain # => "www" |
There was a problem hiding this comment.
| # Addressable::URI.parse("http://www.test.example.co.uk").domain # => "www" | |
| # Addressable::URI.parse("http://www.test.example.co.uk").subdomain # => "www" |
| # Returns the subdomains for this host. | ||
| # | ||
| # @example | ||
| # Addressable::URI.parse("http://www.test.example.co.uk").domain # => ["www", "test"] |
There was a problem hiding this comment.
| # Addressable::URI.parse("http://www.test.example.co.uk").domain # => ["www", "test"] | |
| # Addressable::URI.parse("http://www.test.example.co.uk").subdomains # => ["www", "test"] |
| it "should return an empty array if no subdomain" do | ||
| expect(Addressable::URI.parse("http://example.co.uk").subdomains).to eq([]) | ||
| end | ||
| end |
There was a problem hiding this comment.
Please keep the terminating newline
|
@sporkmonger any thoughts on this? |
This exposes the
PublicSuffix::Domainobject so users can interact with that as needed. Plus it provides a consistent interface for Addressable::URI to interact with.This is useful for accessing subdomains. PublicSuffix's
#trdmethod returns"www.test"from"www.test.example.org".I'd also like to add
subdomain&subdomainsmethods to Addressable::URI for direct access to subdomains (which I have a follow up Pull Request for if this is accepted).