-
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
SerializableResource handles no serializer like controller #1616
SerializableResource handles no serializer like controller #1616
Conversation
4a98876
to
277c5a3
Compare
# For compatibility with the JSON renderer: `json.to_json(options) if json.is_a?(String)`. | ||
# Otherwise, since `serializable_resource` is not a string, the renderer would call | ||
# `to_json` on a String and given odd results, such as `"".to_json #=> '""'` | ||
serializable_resource.adapter.is_a?(String) ? serializable_resource.adapter : serializable_resource |
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.
Does that mean that someone would try to serialize a string directly?
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.
No... as per the comment, this handles render {foo: :bar}.to_json
@groyoh I don't have these failures locally. It makes it hard for me to debug |
@bf4 yeah, I actually tried it it out too earlier. Restarted the build too and couldn't figure out what's going on. |
277c5a3
to
84197e4
Compare
c206c7c
to
6a60d45
Compare
@groyoh I believe it's related to the notifications being passed a nil serializer since that can now happen |
6a60d45
to
ec5dc49
Compare
Fixes when passing in a resource to SerializableResource outside of a controller, a
resource without a known Serializer will no longer raise
NoMethodError: undefined method 'new' for nil:NilClass
, but will behave like thecontroller and just use the resource, as is.
Follows #1580 (comment)