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

Router scope #150

Closed
wants to merge 1 commit into from
Closed

Router scope #150

wants to merge 1 commit into from

Conversation

tmattio
Copy link
Collaborator

@tmattio tmattio commented Jun 1, 2020

This PR adds a Router.scope function to easily build routers where different middlewares are applied to the routes.

This allows to add routes to the router with the following syntax:

let () =
  Router.scope
    ~middlewares:[ Middleware.require_auth ]
    ~route:"/users"
    router
    [ `POST, "/register", Handlers.register_user
    ; `POST, "/login", Handlers.login_user
    ]

@rgrinberg
Copy link
Owner

The name scope does not really fit IMO. What about prefix?

Also, why do we need to combine adding multiple middleware to prefixing routes? Can't these be separate functions?

@tmattio
Copy link
Collaborator Author

tmattio commented Aug 30, 2020

@rgrinberg prefix would imply that the function's main purpose is to prefix a list of routes with a string, whereas the goal is really to "scope" a list of routes by adding a prefix and a list of middlewares to process the requests. Actually, I think the route parameter should be named prefix.

A common use case for this function is an admin interface. You'd want to prefix all the routes with something like /admin and process all the requests for these routes with an authentication middleware.

This could be split into two functions, but I think the use case is common enough that it makes sense to have a function that does both at the same time. The parameters are optional, so one can only prefix routes by not passing a middleware, or only scoping routes with middlewares by not passing route.

The name "scope" is borrowed from Elixir's Phoenix, where a typical route looks like this:

  scope "/", DemoWeb do
    pipe_through [:browser, :redirect_if_user_is_authenticated]

    get "/users/register", UserRegistrationController, :new
    ...
  end

  scope "/", DemoWeb do
    pipe_through [:browser, :require_authenticated_user]

    get "/users/settings", UserSettingsController, :edit
    ...
  end

  scope "/", DemoWeb do
    pipe_through [:browser]

    delete "/users/logout", UserSessionController, :delete
    ...
  end

If you'd still want to split them into two functions, maybe the API could look like this:

let router =
  [ `POST, "/register", Handlers.register_user
  ; `POST, "/login", Handlers.login_user
  ]
  |> Router.with_prefix "/"
  |> Router.with_middlewares [ User_auth_middleware.require_authenticated_user ]
  |> Router.add_routes router

But TBH I find this API a bit confusing.

@tmattio
Copy link
Collaborator Author

tmattio commented Sep 11, 2020

@rgrinberg Here are some alternative APIs for this, let me know if one of them makes sense to you 🙂

Separated functions

let router =
  let open Router in
  [ get "" Handler.dummy
  ; get "/:id/settings" Handler.dummy
  ; get "/new" Handler.dummy
  ; get "/:id" Handler.dummy
  ]
  |> with_prefix "/projects"
  |> with_middlewares Middleware.[ require_authenticated_user; fetch_current_user ]
  |> add_routes
;;

Arguments to add_routes

let router =
  let open Router in
  add_routes
    ~prefix:"/projects"
    ~middlewares:Middleware.[ require_authenticated_user; fetch_current_user ]
    [ get "" Handler.dummy
    ; get "/:id/settings" Handler.dummy
    ; get "/new" Handler.dummy
    ; get "/:id" Handler.dummy
    ]
;;

Scope type

let scope =
  Router.scope
    ~prefix:"/projects"
    ~middlewares:Middleware.[ require_authenticated_user; fetch_current_user ]
;;

let router =
  [ get "" Handler.dummy
  ; get "/:id/settings" Handler.dummy
  ; get "/new" Handler.dummy
  ; get "/:id" Handler.dummy
  ]
  |> Router.add_routes ~with_scope:scope
;;

@rgrinberg
Copy link
Owner

I'll let you make the call. Tbh, I'm not sure what's a user friendly naming convention here. Let me just make a couple of general points:

  • I prefer combinator based API's that do a single thing well.
  • If your API allows you to do the same thing in more than one way without a clear trade off, it's usually bloated.

@tmattio tmattio mentioned this pull request Sep 25, 2020
4 tasks
@tmattio
Copy link
Collaborator Author

tmattio commented Nov 13, 2020

Closing this now, we can continue the discussion in #215

@tmattio tmattio closed this Nov 13, 2020
@tmattio tmattio deleted the router-scope branch December 3, 2020 10:19
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.

2 participants