Use a class for the application routes

[waiting-for-dev]

We move the definition of routes as a block provided to
Hanami::Application.routes to its own class
Hanami::Application::Routes.

Users need to create a subclass and define the routes through a block
applied to the .routes class method. As before, the hanami application
captures the context of the block and forwards it to
Hanami::Application::Router for its evaluation.

E.g.:

require "hanami/application/routes"

module MyApp
  class Routes < Hanami::Application::Routes
    define do
      slice :main, at: "/" do
        root to: "home.show"
      end
    end
  end
end

For consistency with settings, we move the routes path configuration to
the main Hanami::Configuration class.

[jodosha]

@waiting-for-dev LGTM 👍

[solnic]

@waiting-for-dev could we have the DSL at the class level so that there's no need to use routes block within the class?

[waiting-for-dev]

@waiting-for-dev could we have the DSL at the class level so that there's no need to use routes block within the class?

I agree that it'd look nicer. In that case, probably the simplest solution involves letting the user inherit from Hanami::Application::Router and moving those scope, slice, etc., methods to the class level. So the capture of the block of routes and the router wouldn't be in separated classes anymore. Are we ok with that?

[waiting-for-dev]

@waiting-for-dev could we have the DSL at the class level so that there's no need to use routes block within the class?

@solnic, I've been looking into it, and it's not that easy. We are now just capturing the block and forwarding it to a Hanami::Application::Router instance, which executes it in its context. We need to provide a block to a method for that, as we can't just capture the class definition context.

As to using Hanami::Application::Router directly, it's not easy if we want users to interact with it at the class level (in contrast to an instance of it). Take the scope method, it ends up calling super, which belongs to hanami/router, so we can't just move those methods at the class level. A workaround could redefine those methods (scope, slice, etc.) at the class level to build an AST so that later they are executed in the same order at the instance level. We must be sure that it's worth the effort, though. Thoughts?

[solnic]

@waiting-for-dev isn't it possible to create a router instance at a class level and forward DSL methods to it?

[waiting-for-dev]

@waiting-for-dev isn't it possible to create a router instance at a class level and forward DSL methods to it?

Yup, you're right. We can do it. Please, take a look at the last commit.

However, I feel a bit uneasy moving such a thing to the global state. WDYT?

If you think it's a better idea, I'll polish everything and add tests to it.

[jodosha]

@waiting-for-dev The code has been changed since my last review. 🙂

I'm concerned that we're complicating the loading process to save us from the ugly routes block.

---

Remember that we're doing this change not because it's needed but because of syntax consistency with application settings.

It's fine to have consistency, but if that has to come at the cost of complexity, we should stop and reflect.

I agree that routes is ugly, but better having it, than complicating our lives. 😃

/cc @solnic

[jodosha]

@waiting-for-dev Please remove those middleware from here and add them to the Rack middleware stack of the application.

In depth explanation. We can have three nested layers of Rack middleware stacks. Here's them listed from the outermost to the innermost:

• config.ru
• Application
• Router

The less stack we setup the better, for the app performance. When adding default middleware to the stack, we should target Application.

[waiting-for-dev]

I think it's food for another PR 🙂

[jodosha]

@waiting-for-dev ||= isn't thread-safe, is this code executed within a context that guarantees thread-safety?

This configuration heavily depends on global state of Hanami.application. How do we ensure that all these components are ready when we're instantiating the router? Shouldn't this be the role of the caller instead?

[waiting-for-dev]

Not a concern in the new implementation

[waiting-for-dev]

@waiting-for-dev The code has been changed since my last review.

Yeah, @jodosha, the last commit is more like a POC so that we can see how it'd look if we take it that way. I agree with your concerns, so I think that something like a routes method (we could rename it) is the less worst option here.

[solnic]

Can we then rename it to define? The combination of Routes class name and then routes block method name is not 🧊 🙂

[waiting-for-dev]

Can we then rename it to define? The combination of Routes class name and then routes block method name is not

Hey @solnic, I agree with you. It's done now. I'm still leaving the .routes method for the reading operation, though, so we no call it .define 🙃

@jodosha, could you please review it again? @timriley I'd also like to have your eyes on it 🙂

[jodosha]

@waiting-for-dev Thank you very much! LGTM 👍

[timriley]

Looks great! Can't wait to see what we can do with this object now that we have an actual class backing it.