Fixes issue with jsonapi-serializable no longer calling to_json #26
Conversation
lib/jsonapi/rails/renderer.rb
Outdated
| @@ -33,6 +33,7 @@ def rails_renderer(renderer) | |||
| reverse_mapping = request.env[ActionController::REVERSE_MAPPING_KEY] | |||
| options = options.merge(_reverse_mapping: reverse_mapping) | |||
| json = renderer.render(json, options) unless json.is_a?(String) | |||
| json = json.to_json unless json.is_a?(String) | |||
There was a problem hiding this comment.
How about json = renderer.render(json, options).to_json unless json.is_a?(String) instead?
There was a problem hiding this comment.
This works for the master version of jsonapi-serialization, but doesn't with the latest released one. This keeps the gems more independent.
There was a problem hiding this comment.
@beauby, won't this break code which is pinned to the previous versions of jsonapi-serializable (before .to_json was removed)? If renderer.render returns a string, calling .to_json on it will result in an extra round of escaping.
There was a problem hiding this comment.
You guys are obviously right. How about pinning jsonapi-serializable to >= 0.1.3?
There was a problem hiding this comment.
How about pinning
jsonapi-serializableto>= 0.1.3?
I like this better than having an extra .is_a?(String) check.
|
The idea is that this behavior gets streamlined in the next release so we
might as well not check workaround code in.
On Tue, 6 Jun 2017 at 22:56, Sergei Maximov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/jsonapi/rails/renderer.rb
<#26 (comment)>
:
> @@ -33,6 +33,7 @@ def rails_renderer(renderer)
reverse_mapping = request.env[ActionController::REVERSE_MAPPING_KEY]
options = options.merge(_reverse_mapping: reverse_mapping)
json = renderer.render(json, options) unless json.is_a?(String)
+ json = json.to_json unless json.is_a?(String)
How about pinning jsonapi-serializable to >= 0.1.3?
I like this better than having an extra .is_a?(String) check.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACHPYtsvZ8_a1wm90-aXssuHlXhxrHStks5sBb0EgaJpZM4NxCNc>
.
--
Lucas Hosseini
[email protected]
|
There was a problem hiding this comment.
Let's go with json = renderer.render(json, options).to_json unless json.is_a?(String) and pinning jsonapi-serializable in the gemspec.
Would you mind taking care of it @mmuehlberger? Otherwise I'll do it myself tomorrow, as this is probably impacting all new users.
|
@beauby Sure, I'm going to remove the |
|
I understand your concern about the additional layer of dependencies, however I will soon move some bindings into |
|
Actually, after investigation, it turns out that the change in |
|
No, the issue was with the I was using |
|
Finally merged, thanks again @mmuehlberger. |
* upstream/master: Improve specs (jsonapi-rb#33) Expose deserialization reverse mapping in controllers (jsonapi-rb#29) Update jsonapi-rb to v0.2.1. (jsonapi-rb#32) Add dummy app temp files to .gitignore (jsonapi-rb#30) Deal with jsonapi-serializable no longer calling to_json (jsonapi-rb#26)
This is a very simple implementation, and you might have something else in mind, but it fixes the issue of
jsonapi-serializableno longer calling.to_jsonas of jsonapi-rb/jsonapi-serializable#56.Tests pass for currently released versions, as well as
master.Not having this, breaks any requests in Rails that use
to render data.