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

Makes serializer available to resource meta blocks #1960

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@groyoh
Member

groyoh commented Oct 27, 2016

Purpose

Resolves #1940.

It is now possible to access the serializer from the
resource meta block e.g.

class FooSerializer
  meta do |serializer|
    { foo: serializer.foo! }
  end

  def foo!
    "foo!"
  end
end

Changes

Pass serializer to the meta block.

Caveats

This does not provide instance_options to the meta block as instance_options are protected attributes.

Related GitHub issues

#1940

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Oct 27, 2016

@groyoh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rafael, @remear and @bf4 to be potential reviewers.

mention-bot commented Oct 27, 2016

@groyoh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rafael, @remear and @bf4 to be potential reviewers.

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Oct 27, 2016

Member

@groyoh is it not already? (without using the block argument?)

Member

bf4 commented Oct 27, 2016

@groyoh is it not already? (without using the block argument?)

@groyoh

This comment has been minimized.

Show comment
Hide comment
@groyoh

groyoh Oct 27, 2016

Member

@bf4 unfortunately no. Have a look at https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model_serializers/adapter/json_api/meta.rb#L33. We have access to scope and object but nothing else. There are a lot of inconsistencies like this throughout the API though.

Member

groyoh commented Oct 27, 2016

@bf4 unfortunately no. Have a look at https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model_serializers/adapter/json_api/meta.rb#L33. We have access to scope and object but nothing else. There are a lot of inconsistencies like this throughout the API though.

@@ -18,7 +18,7 @@ def initialize(serializer)
# Use the return value of the block unless it is nil.
if serializer._meta.respond_to?(:call)
@value = instance_eval(&serializer._meta)

This comment has been minimized.

@NullVoxPopuli

NullVoxPopuli Nov 13, 2016

Contributor

as a side note, should meta inherit from the same struct we're using elsewhere? would that clean up some of this code?

@NullVoxPopuli

NullVoxPopuli Nov 13, 2016

Contributor

as a side note, should meta inherit from the same struct we're using elsewhere? would that clean up some of this code?

This comment has been minimized.

@groyoh

groyoh Nov 13, 2016

Member

What do you mean exactly? Which struct are you talking about?

@groyoh

groyoh Nov 13, 2016

Member

What do you mean exactly? Which struct are you talking about?

This comment has been minimized.

@NullVoxPopuli

NullVoxPopuli Nov 14, 2016

Contributor

lib/active_model/serializer/field.rb.

Attribute, and Reflection inherit from it.

@NullVoxPopuli

NullVoxPopuli Nov 14, 2016

Contributor

lib/active_model/serializer/field.rb.

Attribute, and Reflection inherit from it.

This comment has been minimized.

@groyoh

groyoh Nov 14, 2016

Member

Not sure how it would benefit from inheriting from Field. But I can definitely see a common behavior that could be reuse with:

  • the block instance_exec
  • the :if, :unless conditionals

I guess I could try something out in a different PR and see if I can come up with something clean.

@groyoh

groyoh Nov 14, 2016

Member

Not sure how it would benefit from inheriting from Field. But I can definitely see a common behavior that could be reuse with:

  • the block instance_exec
  • the :if, :unless conditionals

I guess I could try something out in a different PR and see if I can come up with something clean.

This comment has been minimized.

@NullVoxPopuli

NullVoxPopuli Nov 14, 2016

Contributor

Yeah, a different PR would probably be better, as this pr solves the immediate issue, and what I'm suggesting is more structural.

@NullVoxPopuli

NullVoxPopuli Nov 14, 2016

Contributor

Yeah, a different PR would probably be better, as this pr solves the immediate issue, and what I'm suggesting is more structural.

@groyoh

This comment has been minimized.

Show comment
Hide comment
@groyoh

groyoh Nov 13, 2016

Member

As a side note, I actually think that we should update all the instance_eval to instance_exec(serializer) and align what methods (object, scope, etc.) are available.

Member

groyoh commented Nov 13, 2016

As a side note, I actually think that we should update all the instance_eval to instance_exec(serializer) and align what methods (object, scope, etc.) are available.

@groyoh

This comment has been minimized.

Show comment
Hide comment
@groyoh

groyoh Dec 30, 2016

Member

@NullVoxPopuli I reworked the tests according to your comment 😉

Member

groyoh commented Dec 30, 2016

@NullVoxPopuli I reworked the tests according to your comment 😉

@NullVoxPopuli

This comment has been minimized.

Show comment
Hide comment
@NullVoxPopuli

NullVoxPopuli Dec 31, 2016

Contributor

I approve. As this is a bit of an API change, @bf4?

Contributor

NullVoxPopuli commented Dec 31, 2016

I approve. As this is a bit of an API change, @bf4?

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Dec 31, 2016

Member
Member

bf4 commented Dec 31, 2016

@groyoh

This comment has been minimized.

Show comment
Hide comment
@groyoh

groyoh Jan 1, 2017

Member

@bf4 then please stop reading your mails and enjoy your holidays (?) 😉

Member

groyoh commented Jan 1, 2017

@bf4 then please stop reading your mails and enjoy your holidays (?) 😉

@bf4

Could use a more realistic example and better align with existing impls in lib of same feature

Makes serializer available to resource meta blocks
It is now possible to access the serializer from the
resource meta block e.g.

class FooSerializer
  meta do |serializer|
    { actions: serializer.actions }
  end

  def actions
    instance_options[:actions]
  end
end
@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Jan 3, 2017

Member
Member

bf4 commented Jan 3, 2017

@groyoh

This comment has been minimized.

Show comment
Hide comment
@groyoh

groyoh Jan 3, 2017

Member

I think the idea was to use instance_exec and yield the serializer so that it makes the call to the serializer explicit rather than using "some hidden methods that no one knows about". Doing it in such way could also allow us to align the usage of the DSL:

  1. Yielding the serializer so that it is explicit
  2. Calling helper methods only via the serializer's public interface
class UserSerializer
  meta do |serializer|
    { actions: serializer.actions } # everything goes through the serializer's public API
  end

  link :self do |serializer|
    href serializer.url_for(:self)  # everything goes through the serializer's public API
  end

  attribute :fullname do |serializer|
    user = serializer.object
    "#{user.first_name} #{user.last_name}"
  end

  has_many :posts do |serializer, association_serializer|
    # you get the idea
  end

  def actions # Public method
    # Some actions
  end

  def url_for(type) # Public method
    # A url
  end
end

To me this seems less error-prone and easier to understand than having a few methods available within the block.

Member

groyoh commented Jan 3, 2017

I think the idea was to use instance_exec and yield the serializer so that it makes the call to the serializer explicit rather than using "some hidden methods that no one knows about". Doing it in such way could also allow us to align the usage of the DSL:

  1. Yielding the serializer so that it is explicit
  2. Calling helper methods only via the serializer's public interface
class UserSerializer
  meta do |serializer|
    { actions: serializer.actions } # everything goes through the serializer's public API
  end

  link :self do |serializer|
    href serializer.url_for(:self)  # everything goes through the serializer's public API
  end

  attribute :fullname do |serializer|
    user = serializer.object
    "#{user.first_name} #{user.last_name}"
  end

  has_many :posts do |serializer, association_serializer|
    # you get the idea
  end

  def actions # Public method
    # Some actions
  end

  def url_for(type) # Public method
    # A url
  end
end

To me this seems less error-prone and easier to understand than having a few methods available within the block.

@groyoh groyoh closed this Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment