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

Match against host #104

Merged
merged 2 commits into from May 20, 2016

Conversation

@nicolaracco
Copy link
Contributor

nicolaracco commented Apr 11, 2016

This way it's possible to mount different containers on different domains. E.g.:

Hanami::Container.configure do
  mount Admin::Application, at: '/', host: 'admin.hanami.dev'
  mount Web::Application, at: '/'
end
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 11, 2016

Coverage Status

Coverage increased (+0.5%) to 99.581% when pulling 4058d5f on nicolaracco:host-constraint into ef1a77a on hanami:master.

@@ -143,7 +143,7 @@ def options(path, options = {}, &blk)
# @since 0.1.1
# @api private
def mount(app, options)
add("#{ options.fetch(:at) }*").to(
add("#{ options.fetch(:at) }*", host: options[:host]).to(

This comment has been minimized.

Copy link
@jodosha

jodosha Apr 15, 2016

Member

@nicolaracco Using :host option both as Router#initialize and as a #mount option can be confusing. What about :subdomain instead?


it 'matches against host' do
@router_container = Hanami::Router.new(scheme: 'https', host: 'hanami.test', port: 443) do
mount Front::App, at: '/front', host: 'www.hanami.test'

This comment has been minimized.

Copy link
@jodosha

jodosha Apr 15, 2016

Member

@nicolaracco What happens if a developer only uses the subdomain? Eg. host: 'www'.

This comment has been minimized.

Copy link
@nicolaracco

nicolaracco Apr 15, 2016

Author Contributor

The main problem is that this host parameter actually matches the exact host. It's not clearly documented in the http_router gem but I found it looking at the tests. Specifying the host parameter will actually call the Node#add_host method that relies on the Node::Host class. This class doesn't override the to_code method (defined on Node::AbstractRequestNode) that, by default, seems to execute a simple equality comparison.

I was thinking to do a PR on http_router gem in the future, so that the host param can work against a regexp.

This comment has been minimized.

Copy link
@jodosha

jodosha May 20, 2016

Member

@nicolaracco I see and that is really annoying. I'm gonna merge this, but after Hanami 1.0, we should revise this problem.

@app = Rack::MockRequest.new(@router_container)
response = @app.get('https://www.hanami.test/front/home', lint: true)
response.body.must_equal 'front'
response = @app.get('/front/home', lint: true)

This comment has been minimized.

Copy link
@jodosha

jodosha Apr 15, 2016

Member

@nicolaracco Shall we test this like this? @app.get('https:/hanami.test/front/home', lint: true)

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Apr 15, 2016

@nicolaracco Thanks for this PR. This feature was missing and people kept asking about it from time to time. I left a few inline comments.

@jodosha jodosha self-assigned this Apr 15, 2016
@jodosha jodosha added this to the next milestone Apr 15, 2016
@jodosha jodosha added the enhancement label Apr 15, 2016
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 15, 2016

Coverage Status

Coverage increased (+0.002%) to 99.087% when pulling 6bc9215 on nicolaracco:host-constraint into ef1a77a on hanami:master.

@jodosha jodosha merged commit 6bc9215 into hanami:master May 20, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 99.087%
Details
@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented May 20, 2016

@nicolaracco Merged, thank you! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.