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

README claims that Rack::Request can be used interchangeably with rack env hash, but that does not seem to be true. #41

Open
lasso opened this issue Apr 10, 2015 · 2 comments

Comments

@lasso
Copy link
Contributor

lasso commented Apr 10, 2015

If I use rack's env hash directly everything is ok:

working_app = lambda do |env|
  router = HttpRouter.new
  router.add('/').to(:main)
  router.add('/foo').to(:foo)
  router.add('/foo/bar').to(:foobar)
  matching_routes = router.recognize(env)
  return Rack::Response.new('Not found', 400) if matching_routes.first.nil?
  Rack::Response.new(matching_routes.first.first.route.dest.inspect)
end

run working_app

If I go to '/' I get :main
if I go to '/foo/' I get :foo
If I go to '/foo/bar' I get :foobar

However, if I use a Rack::Request object instead of the env hash things does not work as expected:

non_working_app = lambda do |env|
  router = HttpRouter.new
  router.add('/').to(:main)
  router.add('/foo').to(:foo)
  router.add('/foo/bar').to(:foobar)
  # Wrapping env in a Rack::Request
  matching_routes = router.recognize(Rack::Request.new(env))
  return Rack::Response.new('Not found', 400) if matching_routes.first.nil?
  Rack::Response.new(matching_routes.first.first.route.dest.inspect)
end

run non_working_app

If I go to '/' I get :main
if I go to '/foo/' I get :main
If I go to '/foo/bar' I get :main

The README suggests that the env hash and Rack::Request objects can both be used by the recognize method, but that does not seem to be true. I don't know if it is a real bug or if the docs are simply wrong (or maybe I am doing something wrong), but I'd thought it would be a good thing to report this inconsistency anyway.

@lholden
Copy link

lholden commented Jun 9, 2015

Hi there! Neither Josh or myself have really touched this project in a really long time... so it's always possible that things have changed since the README was written.

If you fix this on your end... feel free to make a pull request and I'll accept it.

@lasso
Copy link
Contributor Author

lasso commented Jun 29, 2015

Hi!

Sorry for the late reply. I haven't really touched my router code since I filed this issue. It was trivial for me to change the code to use the rack environment directly instead of a Rack::Request object.

I don't think there is much demand for supporting Rack::Request objects in addition to the rack env hash, so the simplest solution would probably be to update the README to say that a hash is expected. I'll do a MR for the docs shortly.

lasso added a commit to lasso/http_router that referenced this issue Jun 29, 2015
Update the docs to match discussion in joshbuddy#41.
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

No branches or pull requests

2 participants