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

Representing objects into objects using Representable::Object#to_object #267

Open
elShiaLabeouf opened this issue Jun 22, 2023 · 8 comments

Comments

@elShiaLabeouf
Copy link

Hello @apotonick and the Trailblazer team!

First of all, I'd like to thank you all for the great framework you created and that you've been supporting it for all these years. It's truly great and I appreciate the work.

I met Trailblazer on an API project in 2019 and worked for 2 years with it back then.

Currently I'm working on a legacy SSR Rails 5 application with fat models and controllers and I really want to incorporate Trailblazer there to structure and organize the code.

During my first steps of trying it out on a SSR app, I came to think that it could've come in handy if Representable could make objects as output, i.e.

animal = Animal.new(name: "Barrel")
represented = AnimalRepresenter.new(animal).to_object
represented.name  # => "Barrel"

So that properties could be accessed via method calls.

Why would it be handy? For Rails view helpers, which often take an object and method names it should respond to. Plus partials would look less polluted (on a scale of hundreds variable calls in a single page) comparing to hash key-value usage:

mylongvariable["mylongerylongkey"]
# vs
mylongvariable.mylongerylongkey

I saw some developments in this direction in the Representable source code, but I couldn't make it work like in the example above. And there's no info about this mysterious to_object in the Trailblazer docs.

I decided to create a PR that adds such a feature. The usage is similar to using to_json or to_hash format options. I used Ruby Struct for represented object containers.

I would also love to add a notion about this new option to the Trailblazer documentation as well if the PR gets to master.

Looking forward to hearing your thoughts about it!

@apotonick
Copy link
Member

Hey @elShiaLabeouf, thanks for your kind words! It means a lot to us knowing our tools are helpful to people!

What I am wondering is, what is the matter with the original implementation? What exactly wasn't working for you? We might have missed something, I don't remember ever anyone using the object transformation, so I'm happy you took over!

@elShiaLabeouf
Copy link
Author

Thanks for a quick response! To explain with a few more details:

What I'm trying to achieve

Let's say I have a Rails model Animal that describes a table animals(id, name, age). And I have AnimalRepresenter:

class AnimalRepresenter < Representable::Decorator
  include Representable::Hash

  property :name, getter: ->(represented:, **) { represented.name.upcase }
  property :age,
  property :is_favorite, getter: ->(options:, **) { options[:current_user].favorite_animals_ids.include?(id) }
end

Normally and habitually I'd represent an AR record like this:
represented = AnimalRepresenter.new(animal).to_hash(current_user: current_user)
and it works. I get a decorated object - a hash - with a new represented["is_favorite"] field and the id value is excluded.
But I wish represented was not a hash but an object with properties accessible via method calls: represented.is_favorite

What I tried

I tried to change include Representable::Hash to include Representable::Object, because their source code seemed similar to me, although the docs didn't mention this feature. But with this code:

require 'representable/object'
class AnimalRepresenter < Representable::Decorator
  include Representable::Object

  property :name, getter: ->(represented:, **) { represented.name.upcase }
  property :age,
  property :is_favorite, getter: ->(options:, **) { options[:current_user].favorite_animals_ids.include?(id) }
end

I'm getting an error in the moment of registering the class:

representable/lib/representable/cached.rb:7:in `block in build_definition': undefined local variable or method `format_engine' for AnimalRepresenter:Class (NameError)

          binding_builder = format_engine::Binding
                            ^^^^^^^^^^^^^

I dug the Internet and the Zulip chat to find an example, but unsuccessfully. I decided to fork the repo and make an attempt to implement the feature. Thankfully, the structure of the gem is flexible and easy to extend. So I added a few changes inspired by the hash implementation and added some tests.

Then I noticed the code in test/examples/object.rb, ran it but it resulted with the same error I had got earlier (undefined format_engine). Then I found test/object_test.rb and figured out that the code for my initial task should look something like that:

require 'representable/object'
module AnimalRepresenter 
  include Representable::Object

  property :name, getter: ->(represented:, **) { represented.name.upcase }
  property :age,
  property :is_favorite, getter: ->(options:, **) { options[:current_user].favorite_animals_ids.include?(id) }
end

and with usage like this: AnimalRepresenterModule.prepare(animal).to_object(current_user: current_user).
The getter: options didn't work, so I had to strictly follow the style from the spec: instance: ->(options) { options[:fragment].name.upcase!; options[:fragment] } . To be honest, it looks greek to me.

What I achieved

With the changes from my PR, the line from my initial goal now works: represented = AnimalRepresenter.new(animal).to_object(current_user: current_user).
I get a decorated AR record of a Struct form with custom properties and current user context. The format of the method chain is unified with to_hash and to_json, which is well-documented in the docs. I also paid close attention to the memory usage by caching the Struct class on a representer class level. Plus implemented for_collection and wrap: option.

A brief example of the output from my console:

3.1.0 :009 > animal_array = [Animal.new('Shepard',22,'s'),Animal.new('Pickle',12,'c'),Animal.new('Rodgers',55,'e')]
 => 
[#<Animal:0x00007feaab58a6b8 @age=22, @name="Shepard", @species="s">,
...                                                            
3.1.0 :010 > array_repr = AnimalRepresenter.for_collection.new(animal_array).to_object(wrap: "wrapper")
 => #<struct  wrapper=[#<struct  wrapper=#<struct  name="SHEPARD", age=22>>, #<struct  wrapper=#<struct  name="PICKLE", age=12>>, #<struct  wrapper=#<struct  name="RODGERS", age=55... 

@yogeshjain999
Copy link
Member

@elShiaLabeouf IMO, the to_object method is intended to fill up an object of the same instance and not any other due to it's nature i.e. decorate given object as something else but with a same class.

In your case where you want to access keys of an output hash as a method, could you pass it to OpenStruct for simple method access or Mash if you want nested method access ?

@apotonick
Copy link
Member

@yogeshjain999 Hm, I kind of had the same in mind what @elShiaLabeouf is asking for. If I remember correctly, the object strategy was meant for nested objects. The OpenStruct approach would only work for one level. To me it sounds like a reasonable approach to make to_object work.

@elShiaLabeouf
Copy link
Author

In your case where you want to access keys of an output hash as a method, could you pass it to OpenStruct for simple method access or Mash if you want nested method access ?

Hi @yogeshjain999 ! That's, for sure, one of workarounds to my needs. But if I did so, I presume, the usage of memory would be inefficient since I'd create a hash first and only then convert it to OpenStruct or Hashie resulting in 2x time and resources needed.

Meanwhile, with my approach we directly create a Struct with no middle-man. In fact, Struct is a structure with high performance - 3 to 50 times faster than OpenStruct (benchmark1, benchmark2). So I believe the feature is worth the hustle :)

@elShiaLabeouf
Copy link
Author

@apotonick thank you for having an understanding of the subject! I'd be happy to see it in master!

I have changed the test according to you suggestion. Is there anything else needed to have it merged?

@seuros
Copy link
Member

seuros commented Jul 3, 2023

I just commented on the PR. @elShiaLabeouf Just do it

@elShiaLabeouf
Copy link
Author

Nothing is impossible haha

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

4 participants