Skip to content

Conversation

@povilasjurcys
Copy link

@povilasjurcys povilasjurcys commented Nov 4, 2020

What is the current behavior?

Currently, we can set the relationship id_method_name only as a symbol or string. This does not work well when you want to use custom id value, because you have to create method with same value on object class, aka put serialization logic in to non-serializer class. Example:

class User
  has_many :candies

  def api_candy_ids # I do not want to put this method here - it's used for serialization only!
    candies.map { |candy| "#{candy.id}-nom-nom" }
  end
end

class UserSerializer
  has_many :candies, id_method_name: :api_candy_ids
end

What is the new behavior?

This PR allows to set id_method_name as a lambda/Proc so custom id logic can be defined directly in serializer:

class User
  has_many :candies
  # no need to write #api_candy_ids method here, yay!
end

class UserSerializer
  has_many :candies, id_method_name: -> (user) { user.candies.map { |candy| "#{candy.id}-nom-nom" } }
end

With lambda value, id_method_name key is a bit misleading, but I want to make this change as small as possible. Key renaming, deprecation & co might be done with separate PR.

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

@stas
Copy link
Collaborator

stas commented Nov 4, 2020

@povilasjurcys thank you for the PR, but my current focus is on the #141 where a big rewrite is ongoing.

This covers the use-case you are describing as well here:
https://github.com/jsonapi-serializer/jsonapi-serializer/pull/141/files#diff-d375de8e3ef8cd99b8f0bb7d734098098cbf6c2e253f29efba0f6472bf32d1afR174

Basically, the new version will consider only the ids_method_name for homogeneous collections exclusively. If your collection is mixed, the serializer: -> {} or serializers: { type: Class } option will take over.

This should drastically simplify the API, codebase and maintain the flexibility.

I know the docs are still a bit of an in-progress, but if you could give #141 a test I'll appreciate any feedback.

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants