danott / react-rails-form-helpers

Components for composing forms targeted at Rails
MIT License
81 stars 18 forks source link

Form for nested objects are expected to be scoped with an _attributes suffix #13

Open babgyy opened 6 years ago

babgyy commented 6 years ago

This is a not a bug, merely an open discussion (please tell me if I should do this somewhere else)

When using ActiveRecord's accepts_nested_attributes_for, Rails expects attributes from the nested object to be nested with a _attributes suffix. Using your example (an order has one customer, an order has many burgers), params sent by the form should (as documented) be like this :

{ 
  order: {
    id: 12,
    notes: "A lot of text",
    customer_attributes: {
      name: "Jude",
      email: "hello@example.com"
    }
    burgers_attributes: [
      0: {
        id: 123,
        ...
      },
      1: {
        id: 124,
        ...
      }
    ]
  }
}

HashFields and ArrayFields could be made to automatically suffix their node's name with _attributes. That would however probably break all existing code using this package.

Did you already consider this ? How do you usually use this package for nested associations ?

Thanks again for the package.

danott commented 6 years ago

This is a great place to have an open discussion.

I've considered automatically appending _attributes in the past. The last time I gave it significant thought, I landed on not automatically applying the _attributes suffix. The reasoning I can recall is:

  1. From the outset of this project, I wanted to keep the "magic" to a minimum. This is a hard balance to strike because we're standing on the border between Rails (convention over configuration) and React (composition over convention).
  2. The automatic _attributes suffix requires a little bit of knowledge about the ActiveRecord models' associations. Serializing that association and passing it down as props sounds cumbersome when paired against passing a prop that is name="burgers_attributes" instead of name="burgers".
  3. Some of the largest proponents of Rails cringe at nested attributes. I don't know why this opinion is so prevalent, but there's a recurring theme that nested attributes are great... until they aren't. Personally, I've used them to great success. This is the weakest reason, but the communal distaste does factor into my thinking.

One idea that just occurred to me, is providing an optional boolean prop that defaults to false.

<FieldsFor name="burgers" nested>
  {/* The rest of the form */}
</FieldsFor>

Or even giving it a special component name

<NestedAttributesFor name="burgers">
  {/* The rest of the form */}
</NestedAttributesFor>

This would provide a simple way to opt-in, without breaking existing usage.

What do you think?

babgyy commented 6 years ago

TL;DR I like the idea of an opt-in solution, it would totally work for me ! And I have no personal preference between the two.


  1. From the outset of this project, I wanted to keep the "magic" to a minimum. This is a hard balance to strike because we're standing on the border between Rails (convention over configuration) and React (composition over convention).

I understand and approve this choice. I think it improves adoption and force users to still think carefully about how they build forms. However I was surprised that no one mentioned it and that even the sample doesn't include that. To the point where I checked if Rails 5 changed this behaviour (I am currently on Rails 4.2.4).

Personally, I've used them to great success

Me too !

This is the weakest reason, but the communal distaste does factor into my thinking.

Well without the communal effort, and plugins and gems and so on, my website would be nowhere ! so it's worth taking into account, especially when building a plugin for the community I guess ^^

  1. The automatic _attributes suffix requires a little bit of knowledge about the ActiveRecord models' associations. Serializing that association and passing it down as props sounds cumbersome when paired against passing a prop that is name="burgers_attributes" instead of name="burgers".

Agreed. What pains me, is that IMHO Rails is inconsistent in terms of serialization choices. If I were to serialize your order using as_json method and including associated customer and burgers

class Order < ActiveRecord::Base
  has_many :burgers
  has_one :customer

  def as_json(options={})
    super(:only => [:id, :notes], :include =>[:customer, :burgers])
  end
end

I would get a JSON like this :

{ 
  order: {
    id: 12,
    notes: "A lot of text",
    customer: {
      name: "Jude",
      email: "hello@example.com"
    }
    burgers: [
      0: {
        id: 123,
        ...
      },
      1: {
        id: 124,
        ...
      }
    ]
  }
}

But instantiating an order with this very same JSON would fail to recover the associated records

hash = ActiveSupport::JSON.decode(order.to_json)
Order.new(hash)   # => raise ActiveRecord::AssociationTypeMismatch:

but

hash = ActiveSupport::JSON.decode(order.to_json)
hash["burgers_attributes"] = hash.delete("burgers") if (hash.has_key?("burgers"))
Order.new(hash).burgers   # => works
danott commented 6 years ago

What pains me, is that IMHO Rails is inconsistent in terms of serialization choices. If I were to serialize your order using as_json method and including associated customer and burgers

🤣 at Order.new(ActiveSupport::JSON.decode(order.to_json) raising. I'd never thought about it before, but that definitely feels like a very rough edge of Rails!