hanami / controller

Complete, fast and testable actions for Rack and Hanami
http://hanamirb.org
MIT License
246 stars 111 forks source link

Documentation about Controller/Router integration is incorrect #427

Open weppos opened 1 year ago

weppos commented 1 year ago

The Hanami::Router integration docs seems to be incorrect. It references two parameters (configuration and namespace) that are not accepted by the current 2.x Hanami::Router version:

https://github.com/hanami/controller/blob/2eb83900e298a03a7dadce9cbd1adef33dfec83e/README.md?plain=1#L884-L905

I also noticed that, across various docs and README, the route definition is sometimes referenced using the notation books#show whereas in other cases is books.show.

I wish I could provide a PR to update the docs, but after a few hours of digging into v2 docs and code, I've yet to figure out how to successfully define an action and reference to it in a router without using the fully qualified to: ClassName.new notation.

weppos commented 1 year ago

I actually feel even more dumb now. The second example in the README of this repo references a Hanami::Controller::Configuration.new object, but I actually cannot find it anywhere in v2. It seems it was in the controller v1.3, but it's entirely gone in v2.

Are the README examples outdated, or am I missing something obvious? 😕

jodosha commented 1 year ago

@weppos Hey Simone, this is a leftover of past WIP implementation. My apologies.

The fix is on the way. Given that I'm going to touch the README, is there anything specific you were looking for?

weppos commented 1 year ago

Hi @jodosha!

The fix is on the way. Given that I'm going to touch the README, is there anything specific you were looking for?

As you recall, we use only the router and controller. I am currently looking for some examples on how to make it happen. A simplified version of the current code looks like:

Hanami::Router.define do
  get     "/accounts", to: "accounts#index"
end

The app itself is the following. Note how the controllers where scoped on a specific namespace.

class App
  def self.routes
    Hanami::Router.new(namespace: Api::V2::Controllers, &eval(File.read('path/to/routes.rb'))
  end

  def initialize
    routes = self.class.routes

    @app = Rack::Builder.new do
      use Hanami::Middleware::BodyParser, :json
      run routes
    end
  end

  def call(env)
    @app.call(env)
  end
end

and each action was

module Api::V2
  module Controllers::Accounts

    class Index
      include Hanami::Action

      def call(_params)
        render "..."
      end
    end

  end
end

There's a number of changed I think we'll have to make. I've noticed all actions are now subclassed from Hanami::Action, and that seems easy enough.

The piece I'm missing is how to initialize a router and connect it so that it resolves the actions. I was able to make this work:

def self.routes
  # Hanami::Router.new(namespace: Api::V3::Controllers) do
  #   get "/whoami", to: "authentication_context.show"
  # end
  Hanami::Router.new do
    get "/", to: ->(*) { [200, {}, ["OK"]] }, as: :welcome
  end
end

but nothing more than that. The following were tests I made and that's when I discovered the various readme issues:

def self.routes
  Hanami::Router.new(namespace: Api::V3::Controllers) do # namespace doesn't exist anymore
    get "/", to: ->(*) { [200, {}, ["OK"]] }, as: :welcome
  end
end

also

require "hanami/controller"

module Api::V3
  module Controllers::AuthenticationContext
    class Show < Hanami::Action
      def handle(*)
        res.format = :json
        res.body = {}
      end
    end
  end
end
def self.routes
  Hanami::Router.new do
    get "/", to: ->(*) { [200, {}, ["OK"]] }, as: :welcome
    get "/test1", to: Api::V3::Controllers::AuthenticationContext::Show.new # error undefined local variable or method `res'
    get "/test2", to: "authentication_context#show" # error undefined method `call' for "authentication_context#show":String
  end
end

I must be missing something obvious to connect a router get to the corresponding action. 😅

jodosha commented 1 year ago

@weppos

error undefined local variable or method `res'

That is due to the definition of Api::V3::Controllers::AuthenticationContext::Show#handle. Your current def is handle(*), so it discards the arguments.

It can accept two args: handle(req, res), for request and response, respectively. You can bind only what is needed in the following variations:

It really depends on what you need.

weppos commented 1 year ago

That is due to the definition of Api::V3::Controllers::AuthenticationContext::Show#handle. Your current def is handle(*), so it discards the arguments.

Good catch! It was a left over from the various attempts I made to implement a working code from the docs 😅 So now I was able to make test1 working, hence the routing works correctly as long as the definition uses a fully qualified instance. I could not figure out how to make so that also the string representation "authentication_context#show" is supported.

jodosha commented 1 year ago

@weppos

One notable change from hanami-router 1 to 2 is that the router doesn't attempt to load the endpoints starting from a string.

That simplified the library: it isn't a loader anymore but only a dispatcher.


But there is a way to solve this problem: Hanami::Router#initialize accepts a keyword argument resolver. It requires an object with the following signature #call(path, to) -> Object.

During the initialization time, for each declared route it gets invoked. You can use it to implement a custom loading policy.

Example:

class MyResolver
  def initialize(namespace: Api::V3::Controllers)
    @namespace = namespace
  end

  def call(path, to)
    puts "path: #{path}"
    puts "to: #{to}"

    class_name = to.classify
    @namespace.const_get(class_name).new
  end
end

router = Hanami::Router.new(resolver: MyResolver.new) do
  get "/test2", to: "authentication_context/show"
end

# => "path /test2"
# => "to authentication_context/show"
weppos commented 1 year ago

OK, thanks. We'll look into it. That's a lot of knowledge that would be nice to have somewhere documented. 😉