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

Replace outdated ActiveModel::Serializers by Panko::Serializer #1139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

casperisfine
Copy link
Contributor

@casperisfine casperisfine commented Feb 15, 2021

Most of the code was super easy to convert.

So far I've found one blocker:

  • There's no way to conditionally include attributes. Best we can do is to have a null value.

I'll see if I can get that feature added upstream.

@casperisfine
Copy link
Contributor Author

  • There's no way to conditionally include attributes. Best we can do is to have a null value.

Upstream accepted the patch very quickly. There is still a minor limitation as it only work for regular attributes, relations can't be skipped. But I think it's probably fine. It just means a couple of API fields will be explicitly null rather than being skipped. I can live with that.

Overall the integration was super easy, I'm just a bit worried because the test coverage of the serializers is relatively limited, so It's not unlikely I missed some difference between the two serailizers.

@casperisfine
Copy link
Contributor Author

Ok, so I found another big issue. Panko directly tries to reach into the Active Record attributes object, which means that overriden attribute methods, or even virtual attributes are not supported, e.g.

class Stack < AR::Base
  def repo_name
    repo.name
  end
end

class StackSerializer << Panko::Serializer
  attributes :repo_name
end

stack.repo_name # => "Some String"
StackSerializer.new.serailize(stack).to_json # => '{"repo_name": null}'

I think I can get this handled upstream, but ultimately I'm getting a bit uncomfortable with the panko_serializer implementation, that's a lot of C code that reach in Active Record internals, I'm kind of worried about its maintainability.

I might look for another gem.

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