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 about Controller/Router integration is incorrect #427

Open
weppos opened this issue Jun 29, 2023 · 7 comments
Open

Documentation about Controller/Router integration is incorrect #427

weppos opened this issue Jun 29, 2023 · 7 comments
Assignees

Comments

@weppos
Copy link
Member

weppos commented Jun 29, 2023

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:

controller/README.md

Lines 884 to 905 in 2eb8390

### Hanami::Router integration
```ruby
require "hanami/router"
require "hanami/controller"
module Web
module Controllers
module Books
class Show < Hanami::Action
def handle(*)
end
end
end
end
end
configuration = Hanami::Controller::Configuration.new
router = Hanami::Router.new(configuration: configuration, namespace: Web::Controllers) do
get "/books/:id", "books#show"
end
```

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
Copy link
Member Author

weppos commented Jun 29, 2023

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 jodosha self-assigned this Jun 29, 2023
@jodosha
Copy link
Member

jodosha commented Jun 29, 2023

@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
Copy link
Member Author

weppos commented Jun 29, 2023

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
Copy link
Member

jodosha commented Jun 29, 2023

@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:

  • handle(*)
  • handle(req, *)
  • handle(*, res)
  • handle(req, res)

It really depends on what you need.

@weppos
Copy link
Member Author

weppos commented Jun 29, 2023

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
Copy link
Member

jodosha commented Jun 29, 2023

@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
Copy link
Member Author

weppos commented Jul 3, 2023

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

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