-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Rendering array objects that doesn't have serializers #962
Changes from all commits
d589268
189b795
3710c32
cf77786
e5d1e40
d3649d5
741c4a4
17d560e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
module ActiveModel | ||
class Serializer | ||
class ArraySerializer | ||
NoSerializerError = Class.new(StandardError) | ||
include Enumerable | ||
delegate :each, to: :@objects | ||
|
||
|
@@ -13,7 +14,12 @@ def initialize(objects, options = {}) | |
:serializer, | ||
ActiveModel::Serializer.serializer_for(object) | ||
) | ||
serializer_class.new(object, options.except(:serializer)) | ||
|
||
if serializer_class.nil? | ||
fail NoSerializerError, "No serializer found for object: #{object.inspect}" | ||
else | ||
serializer_class.new(object, options.except(:serializer)) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about having serializer_for return nil for if any elements don't have an associated serializer? That would maintain current PR behavior, I think, might make more sense SRP-wise, but might be a performance hit. Should we add a test for a collection or association that contains an element with a known serializer and another with a nil serializer class? I'm really not sure how to handle that in AMS without more complexity. I'd rather push that back to the end user, but that could be laziness. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would need to update this PR if we change this behaviour at I think it's an edge-case. I haven't heard about ppl using AMS to renders arrays that mix serializable and non-serializable objects yet, so let's not worry about it for now 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. winning |
||
end | ||
@meta = options[:meta] | ||
@meta_key = options[:meta_key] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add some docs on building your own array serializer as described in https://github.com/rails-api/active_model_serializers#2-for-an-array-resource and the use of this exception class at some point. Want me to make an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on a quest to keep the readme as clean as possible, but I agree that this can be helpful for some ppl, like others details that are poping up.
I'm thinking about start the AMS wiki (using gh wiki feature). For now, make yourself comfortable to open the issue to make sure we won't forget it while I think it trough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like wikis, but then it's another thing to garden. For AMS I think it could be a plus.