hanami / view

Views, templates and presenters for Ruby web applications
http://hanamirb.org
MIT License
173 stars 80 forks source link

POC for hanami components #198

Closed waiting-for-dev closed 2 years ago

waiting-for-dev commented 2 years ago

This is a POC for a different approach to managing how classes within a Hanami application integrate with it. It builds on top of previous work done in https://github.com/hanami/view/pull/173.

Although #173 was a step forward, it's not an ideal solution, yet. The most critical flaw is breaking the separation between a third-party library and the Hanami integration itself. We should see as a coincidence that hanami/view and hanami/controller belong to the same organization that hanami/hanami. I.e., we shouldn't rely on the to-be-integrated code knowing about Hanami.

On top of that, I'd say having a class acting differently when we detect it's within the context of a Hanami application goes against the Hanami less-magic philosophy.

This draft introduces a new Hanami::Component module. When a class extends it, it gets knowledge about the surrounding Hanami application in two different ways:

That's been extracted from Hanami::Application#provider and repetitions made for every repetition of the previous pattern. Making it a first-class module makes what's going on more explicit. On top of that, it leaves the doors open to provide a controlled and limited share of information that we want to make available to other hanami components.

As said, the new solution allows for creating other integrations without monkey patching third-party code. It also gives more flexibility, as users can develop different integrations for the default stack (hanami/view & hanami/controller).

The current implementation has to be seen as a POC. If you agree with taking that direction, I'll polish everything. To begin with, hanami/component needs to go in hanami/hanami. We should also modify hanami/controller. We can go fancier here, caching the #provider method through a module builder.

Notice that the change would be backward incompatible, as the hanami template should inherit from Hanami::View::ApplicationContext and Hanami::View::ApplicationView. However, as I said, I see it as the right thing to do in favor of explicitness.

timriley commented 2 years ago

@waiting-for-dev This is cool! Thanks for putting some time and thought into this. I've definitely been conscious of the repetition and lack of proper encapsulation of this integration concept, so I'm really thankful you found the chance to begin taking this to the next level!

Perhaps the biggest thing to clear up before proceeding would be the end-user experience here, i.e. whether we have users inherit from Hanami::View or Hanami::View::ApplicationView in their applications. When I was doing this work originally (and gosh, it feels so long ago now!), I was guided by a preference from @jodosha that the integration be as "seamless" as possible, i.e. that it be Hanami::View our users inherit from within Hanami applications.

However, a few things have changed since then, such as our application template taking form (which itself shall in turn inform our generators), and a base view class being included in it. So really, we're only talking about a small handful of classes (e.g. one per slice) that will be inheriting from a class within Hanami::View anyway. Given this limits the impact of this change, perhaps @jodosha's perspective will be different at this point.

Another thing that has emerged is the Application<Foo> naming pattern occurring in a few additional places, e.g. in our config/routes.rb as class Routes < Hanami::Application::Routes, as well as some others.

In the end, I think the names probably won't matter so much so long as (a) they're generated automatically for the user by our generator tools (which we plan to do), and (b) they read ok to someone scanning the code of the generated files ("application view" seems fair enough in this case), and (c) we have decent docs about how the integration works (which we can tackle I due course).

So given this, I'm OK with the superclass naming change if it gives us significantly more clarity of internal implementation.

Now, as for the implementation, I feel a little bit on the fence about it. With the structure we have in this PR, we're actually leaving the application and the provider exposed on every subclass of Hanami::View::ApplicationView. This is giving access to objects that the subclasses really shouldn't know anything about, e.g.

module Main
  class MyView < Hanami::View::ApplicationView
    def some_method
      # Look, I have what appears to be "blessed" direct access to Hanami.application
      # without having to access it directly via its constant (which is usually the signal
      # by which you can tell you're reaching too far outside your boundaries)
      self.class.application 

      # Same for the Main::Slice too
      self.class.provider
    end
  end
end

In the original implementation, we avoided this thanks to ApplicationView being a Module subclass in which we generate whatever behaviour we need using the application and provider, but without making them accessible on the including class.

Yes, this required a bunch of metaprogramming and some less-used Ruby techniques compared to what we have in this PR, but IMO it made for a cleaner outcome.

So if we go down this path, I'd really like us to explore if we can avoid us exposing application and provider. If we can achieve that somehow, then we'd be getting the best of both worlds. What do you think, @waiting-for-dev?

waiting-for-dev commented 2 years ago

Many thanks for looking into it @timriley!

I was guided by a preference from @jodosha that the integration be as "seamless" as possible, i.e. that it be Hanami::View our users inherit from within Hanami applications.

Thanks for the historical perspective; it's precious here. Ok, so I'll wait for @jodosha's greenlight before moving forward.

in which we generate whatever behaviour we need using the application and provider, but without making them accessible on the including class.

That's a really good point. I've been thinking a bit about it, as we should consider all the trade-offs.

The most straightforward implementation for that would be calling a hook defined on the including class from Hanami::Component. E.g.:

class Hanami
  module Component
    def self.included(klass)
      klass.configure_for_hanami(provider_for_constant(klass), Hanami.application)
    end
  end
end

class MyComponent
  include Hanami::Component

  def self.configure_for_hanami(provider, application)
    # ...
  end
end

I agree that would be the best solution in terms of encapsulation. However, I don't like that it encourages global state or metaprogramming. That's because we only have access to the component class, so you can only set class variables or use something like #define_method. Obviously, the global state is already there, though, but it'd be too easy to redefine pieces of it on the including class.

Another option is to limit what we're exposing. Instead of making the Hanami application and provider available, we can take fine-grained control over it. For instance, a current pattern is the need to fetch the inflector:

class Hanami
  module Component
    def self.inflector
      Hanami.application.inflector
    end
  end
end

class MyComponent
  extend Hanami::Component
end

I'm not sure whether that would be too limiting, but I don't feel that bad that the including class and its descendants get to know about the inflector.

We could also mix both approaches, and call a hook method if defined, just in case the including class needs to do something fancier.

Thoughts? :slightly_smiling_face:

timriley commented 2 years ago

FWIW, @waiting-for-dev, I found this earlier discussion which might be pertinent: https://trello.com/c/W8n8WdtW/134-should-we-rename-hanami-controller-to-hanami-action#comment-609fc852eca9a17deedd5a76

timriley commented 2 years ago

@waiting-for-dev I think to cater to whatever possible kind of integrations that 3rd party components might desire for their classes, it'd be best just to make the overall provider available to them, rather than specific things, especially if one half of this code ends up sitting inside the hanami gem itself — at that point the hanami gem can't reasonably know what any given 3rd party components might require for its integration.

So given this, I'd lean towards that first option of yours.

As for metaprogramming, I don't mind it in this context. Of course we want to limit it as much as possible, but it does achieve the cleanest outcome here. I'd rather have a metaprogrammed integration than having unnecessary details of the wider application leak out as accessible methods on the class.

Let me know what you think 😄

waiting-for-dev commented 2 years ago

That makes sense, @timriley. I'll definitely give it a try with the first option. Thanks again for your feedback!

solnic commented 2 years ago

Don't you think hanami should generate abstract classes for components and then you just inherit from them + tweak them if needed? So:

module Main
  # Main::ApplicationView is provided via hanami/view integration
  class View < ApplicationView
  end
end

If you want a different name, you could configure it:

class Application < Hanami::Application
  config.view.abstract_class_name = "AbstractView"
end

Personally, I'd love if it worked like that with everything, whenever we need an abstract class. In general whenever I need to inherit from a framework class I feel unease 😉 There are of course many cases where it makes a lot of sense, but if you want to limit how much is exposed, then generating abstract classes with less coupling to a framework is the right thing to do.

waiting-for-dev commented 2 years ago

Hey @solnic, I'm not sure I fully understand your comment. Precisely, the idea in this POC is for integrations to build their own component classes, i.e., you wouldn't need to inherit from a framework class. Hanami::Component is only used by the integrating class to build itself.

That said, I wonder if we're making things more complicated than they should be. As @timriley and @solnic pointed out, the main idea is to limit how much is exposed: we only want the Hanami information to be available during the integration setup. At the same time, we don't want the integrating library to know about Hanami (we want to build an integration, aka. adapter).

Because of that, I proposed to call a hook method when including Hanami::Component. I've been playing with it, and it comes with a few quirks:

Thinking about it twice, if we only want to use and throw away information, what about using a simple collaborator? E.g.:

class Hanami
  class Component
    attr_reader :provider
    attr_reader :application

    def initialize(klass)
      @application = Hanami.application
      @provider = provider_for(klass)
    end

    def provider_for(klass)
      # ...
    end

    def inflector
      application.inflector
    end
  end
end

Then, any integration can use an instance of this class however it wants. As in the example with inflector, we can provide shortcuts for common pieces of information. It won't limit in any way the given amount of flexibility, as provider and application keep being 100% available.

waiting-for-dev commented 2 years ago

Keep thinking about it :slightly_smiling_face:

Given that:

1.- A hanami application is global state. 2.- It's global state that changes over time (1. not inited, 2. inited, 3. booted).

Another more architecturally-sound solution would be to publish those changes in the form of events, and let the hanami components subscribe to them. We could start easily, without the need for external libraries (like dry/events). E.g.:

module Hanami
  def self.subscribe(event, klass, &block)
    @subscribers[event] << [klass, block]
  end

  def self.publish(event)
    @subscribers.each do |(klass, subscriber)|
      subscriber.call(provider_for(klass), application)
    end
  end

  class Application
    def init
      Hanami.publish(:init)
    end
  end
end

class ApplicationView < Dry::View
  Hanami.subscribe(:init, self) do |provider, application|
    # ...
  end
end

It could be helpful in other contexts, like registering internal components like the routes. What bugs me is that we break the typical unique direction of event-driven architecture because of the need to pass klass to Hanami. I wonder if we could avoid that somehow... I'll give some thoughts, but any feedback is welcome :slightly_smiling_face:

solnic commented 2 years ago

Hey @solnic, I'm not sure I fully understand your comment.

@waiting-for-dev ah OK I thought it was related to what you're doing here

solnic commented 2 years ago

@waiting-for-dev I'm having some second thoughts about this. My hope was to avoid having to write special code in order to plug something into hanami because dry-system should provide everything that's needed to add 3rd-party components. I need to revisit how hanami-view/controller is implemented because maybe you're trying to solve a problem that we shouldn't have in the first place.

Related - I've built an advanced component system for rom 6.0 (still a WIP in master) that probably solves what you're trying to solve but again - we shouldn't have such problems in Hanami 2.0 (unless I'm missing something).

waiting-for-dev commented 2 years ago

Closing in favor of https://github.com/hanami/hanami/pull/1156