Add to_component_class to active record base

[vinistock]

Closes #26.

This PR aims to implement the to_component_class which should map a model to its respective component class. Furthermore, it implements a new branch in the render monkey patch to deal with this particular case.

A couple of concerns for feedback:

1. In order to figure out how to map the record's attributes to the component's attributes, I am listing the parameters from the initialize method and slicing the record's attributes. I'm not sure this is the best way forward.
2. Adding active_record to the dummy app forced me to add database.yml. Is there a way to get around that?
3. Should we split the ActiveRecord monkey patch into another file?

[juanmanuelramallo]

This looks amazing! Should we update the readme to include an example of this?

[tomasc]

Thank you for the PR, however please let’s not go this route. Components do not necessarily map to models, in fact their strength is in their conceptual decoupling from models. If this PR is merged a relationship between models and components will be implied, which is a sort antithesis of the whole idea of components.

That being said, if you prefer to use components like this, it should be very easy to implement this in your application, eventually you could make a small gem based on this PR that provides the glue between ActionView::Component and ActiveRecord.

[juanmanuelramallo]

Hey @tomasc, this change is solving issue #26. Which was introduced https://github.com/github/actionview-component/issues/15#issuecomment-524521196.

IMHO

<%= render @book %> 

looks a bit better than

<%= render BookComponent, book: @book %>

IMHO this change won't imply that an ActionView::Component must be coupled to a model. This is just about making components more railsy. Allowing them to work as common action view partials do.

Do you still think this change don't belong here?

[tomasc]

@juanmanuelramallo hmm, if I understand correctly then I still think this should not be merged.

I have previously used Draper which, in a way, does what you are proposing: it decorates models with view-related methods (at least that’s how I used it). Over time I found this very misleading: the views, conceptually, should not mirror models. For example you might have a component called Book, which has title, author and isbn attributes. These however might (and in many cases should) come from different sources: models, API, value objects, what have you.

The PR you are proposing seems Rails-y, mainly because it looks good and obvious, but it would promote architecture that I believe is already supported through standard Rails views. What I like on the ActionView::Component is that it implements a new, imho more modern, architecture in addition to what is already in Rails today (MVC). And by not supporting direct relation between model and component it would be Rails-y, because opinionated.

But let’s have others chime-in. @joelhawksley what do you think?

[vinistock]

I don't think having multiple ways of doing things in Rails should be a concern. That's pretty common in the framework and most alternative implementations don't stop you from using other types.

Having that said, I understand the concern of needing more information that doesn't all come from a single model. We could also augment this to accept extra arguments in the following way:

<%= render @record, some_other_thing: :something %>

Also, you could still use the regular rendering with this implementation, but I do believe it needs to be Railsy if we ever intend to upstream this. It will probably have a much lower chance of being accepted if it doesn't fit the Rails way of doing things.

Additionally, let's say we have an active record relation (say @posts). Do you a suggestion of how we could avoid having these loops all over our views?

<% @posts.each do |post| %>
  <%= render PostComponent, title: post.title %>
<% end %>

With the addition of to_component_class, the rendered can figure it out on its own and it can simplify the loops to simply doing:

<%= render @posts %>

Let me know more of your thoughts.

[tomasc]

Hi @vinistock,

I would propose to extend the standard way of rendering collections in Rails for example like this:

<%= render component: BookComponent, collection: […], locals: {…} %>

Where the collection argument is is an Array of objects that respond to arguments required by the PostComponent (for example :title, :author and :isbn).

As a consequence, this would, again, promote passing specific arguments to a component, instead of simply handing over whole models. It is more verbose, yes, but I don't think the most concise way of writing render statements is the necessary goal here.

[joelhawksley]

@vinistock thank you for taking the time to write this PR, and to @tomasc and @juanmanuelramallo for the spirited debate.

To be honest, I'm pretty torn here.

@tomasc I tend to agree with you when you say:

Components do not necessarily map to models, in fact their strength is in their conceptual decoupling from models. If this PR is merged a relationship between models and components will be implied, which is a sort antithesis of the whole idea of components.

I think it's good to be able to decouple components from models, but I'm increasingly less convinced that this is something ActionView::Component should enforce. In a lot of Rails apps, simply passing a single AR object to a view/component is probably just fine!

If I had to decide, I probably would include this patch, mainly because it further aligns the implementation with the "Railsy" way.

(As an aside, we totally should support render collection: #21)

[vinistock]

I've split the patches and re-ordered the render hierarchy. Let me know if you got other concerns.

Particularly, is there anyway to get rid of database.yml if we're only using sqlite3 in memory?

[kaspermeyer]

Great idea @vinistock! I really like the idea of moving closer to the Rails way of rendering and being able to use the shorthand syntax with components:

<%= render @post %>

My only concern about this, is the fact that we're creating a really tight coupling between the model and the component, when we map all the individual attributes from the model to the component. It's not obvious that you would have to update the parameters on the component initializer every time an attribute is added/removed/renamed on the model itself.

Could we perhaps pass the model instance instead? It would create a much looser coupling and it would mimic the behavior we're used to from partial rendering where <%= render @post %> render the _post.html.erb partial and passes a local named post that refers to the model.

An added bonus of passing the model instead of individual attributes is that any class could implement #to_component_class and it would still work as expected.

[vinistock]

@kaspermeyer I agree that the way of passing the attributes to the component might feel a little awkward. However, I don't believe we should pass the instance.

If we pass it, then it allows for querying inside the components, which I understand is not the desired concept.

For example:

class MyComponent < ActionView::Component
  def initialize(record)
    @stuff = record.more_records
  end
end

We could think of some other way of defining the component's properties that makes extracting those from the model more elegant. I'm open to suggestions.

[joelhawksley]

@vinistock @kaspermeyer IMO we should just pass the instance into the component, as that's how to_partial_path handles it.

I'd prefer we stick to matching existing behavior, at least for now.

[vinistock]

@joelhawksley @kaspermeyer okay. Moved the patch to ActiveModel and started accepting the instance in the component.

[joelhawksley]

This is looking good! Thank you for your work on this ❤️

[joelhawksley]

In what case does the nil? check help us here?

[vinistock]

Because we are adding the to_component_class to every model, we need to check that the model does indeed have a corresponding component class before trying to render it.

It will return nil if the component class doesn't exist for the model.

[joelhawksley]

Looks great! Might we update the readme before merging this in?

[vinistock]

@joelhawksley let me know if you like the copy.

[joelhawksley]

Thanks for yet another wonderful contribution @vinistock! ❤️