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

Documentation on how routes are prioritized #45

Open
hcarty opened this issue Nov 9, 2015 · 5 comments
Open

Documentation on how routes are prioritized #45

hcarty opened this issue Nov 9, 2015 · 5 comments

Comments

@hcarty
Copy link
Contributor

hcarty commented Nov 9, 2015

Given a modified version of the README example:

open Opium.Std

let print_param = get "/hello/:name" begin fun req ->
    `String ("Hello " ^ param req "name") |> respond'
  end

let print_other = get "**" begin fun req ->
    `String "Hello everyone else" |> respond'
  end

let () =
  App.empty
  (* Order matters! *)
  |> print_other
  |> print_param
  (* Order matters! *)
  |> App.run_command

With this code I see "Hello test" if I GET /hello/test and "Hello everyone else" if I use other paths. However, if I swap the lines between the comments above then the only response "Hello everyone else". OCaml 4.02.3 + opium 0.13.3 + cohttp 0.19.3 from opam.

I'm not sure what the right approach is here. It's useful to be able to have specific routes with an catch-all to handle all other requests. It would be useful if the right way to do this were shown in the README or somewhere similarly visible.

@rgrinberg
Copy link
Owner

OK, I see the problem. I think that the ** route should always match last in such a case but as you've said it might be useful to have an explicit catch all route.

@hcarty
Copy link
Contributor Author

hcarty commented Nov 12, 2015

Agreed, ** matching last is what I was hoping for. In general it would be nice if the most precise route were picked. For example the route /hello/test would be selected before /hello/:name which would be selected before **.

@cullophid
Copy link

Could you not just change the order of the routes?
This is the same behaviour you see in sinatra and express.js and generally its quite nice that you have to be specific about what order the routes get evaluated in

@jochasinga
Copy link

Could you not just change the order of the routes?
This is the same behaviour you see in sinatra and express.js and generally its quite nice that you have to be specific about what order the routes get evaluated in

To add to @cullophid I agree it's nice about this explicit ordering plus the |> operator already does a great job of being semantic. An emphasized doc on this is all is needed.
I think adding another way other than using |> for implicit route matching might be appropriate if needed.

@rgrinberg
Copy link
Owner

Will be fixed by #215 . We're not going to allow the default router to have ambiguous routing.

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

4 participants