Pass options into collection blocks

[trevorrjohn]

• this also changes the behavior to instantiate a new serializer for each collection item.
• updates readme to fix styling

[wmakley]

Thanks for pointing out the missing feature! I used your spec and modified it to match the style. I then applied the quick fix, which was just to add an additional argument to an instance_exec.

I'm not merging the README change, because the README is literally correct: The resulting Hash DOES have symbol keys. It only becomes stringified when you use ActiveSupport's to_json or as_json methods. Does that make sense?

Is there a reason you need a new serializer instance for each item in a collection? It might be a micro-optimization, but one of the reasons I created this library is I had performance problems with AMS.

[trevorrjohn]

The reason for the new serializer instance is for a use case where you need to memoize something in the collection. eg:

class Foo < SimpleSerializer
   has_many :users
end

class UserSerializer < SimpleSerializer
  attribute :pending_relationships do |object|
    relationships(object).count { |x| x.pending? }
  end

  attribute :invited_relationships do |object|
     relationships(object).count { |x| x.invited? }
  end

  private

  def relationships(object)
    @relationships ||= object.relationships.to_a
  end
end

Because we are not instantiating a new class, the instance variable will be set to the first users relationships. Maybe there is a better pattern for this???

[trevorrjohn]

Also I think you missed the important part of passing in the options on https://github.com/wmakley/simple_serializer/blob/master/lib/simple_serializer.rb#L205. Without that you don't have the options argument in the block. Unless I am misunderstanding something.

[wmakley]

You're right, it violates expectations not to instantiate a new one in that case. I fixed the code point you mentioned as well.

[trevorrjohn]

Do you want me to submit another PR?

[wmakley]

No, I took care of the options issue already if you grab master. I'm thinking about what to do with the instance issue briefly. The main reason I created this is I wanted to switch to netflix/fast_jsonapi, which has a much more restrictive API, but I guess I don't see a reason not to have a more permissive API if you want to use it as a primary serialization library. I'm not going for 100% compatibility with AMS, just none of the surprises of AMS.

[wmakley]

Here is a branch with the instance change: https://github.com/wmakley/simple_serializer/tree/new-instance-per-item

[trevorrjohn]

I am curious what your thoughts are on this, and why the performance benefits might out weight the benefits of having a consistent api. I just see getting the same class instance back as something that could potentially lead to unintended bugs that are hard to track down.

[wmakley]

I'm not sure what my goals are now, I've already met my needs of not leaking memory (I think) and being simple and easy to invoke instead of using a million enterprise-y adapter classes, and implementing the simpler parts of the AMS API (I never used the stuff I am getting PR's for) while being as performant as I know how to make it.

I want to ensure it continues meeting my needs, but allow it to be useful to other people who used more of the AMS API. I will probably merge this branch, I guess I just want to know if there is a meaningful performance cost for 1000+ records.

[trevorrjohn]

I think that makes sense. Maybe I can come up with some benchmarks as part of this PR.

On Tue, Jul 31, 2018, 17:59 William Makley notifications@github.com wrote:

I'm not sure what my goals are now, I've already met my needs of not leaking memory (I think) and being simple and easy to invoke instead of using a million enterprise-y adapter classes, and implementing the simpler parts of the AMS API (I never used the stuff I am getting PR's for) while being as performant as I know how to make it.

I want to ensure it continues meeting my needs, but allow it to be useful to other people who used more of the AMS API. I will probably merge this branch, I guess I just want to know if there is a meaningful performance cost for 1000+ records.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wmakley/simple_serializer/pull/7#issuecomment-409381633, or mute the thread https://github.com/notifications/unsubscribe-auth/AApILqimdrQxs_gsrJjcnvLtn0dvaMz7ks5uMNMwgaJpZM4VoVOp .

[trevorrjohn]

@wmakley so I created a benchmark with 10k collection objects

https://github.com/trevorrjohn/simple_serializer/blob/tj/benchmark/benchmark.rb

% bundle exec ruby benchmark.rb
Rehearsal ------------------------------------------------------------
existing serializer:       0.039917   0.000798   0.040715 (  0.040712)
new instance serializer:   0.036289   0.001501   0.037790 (  0.037830)
--------------------------------------------------- total: 0.078505sec

                               user     system      total        real
existing serializer:       0.035151   0.001842   0.036993 (  0.037069)
new instance serializer:   0.035960   0.001199   0.037159 (  0.043052)

I am not seeing any major performance degradation with the new implementation.

[wmakley]

Wow, thanks! Posted to master.