-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Json render #88
Json render #88
Conversation
This will introduce some unnecessary dependancies. Most people stopped using custom JSON parsers (like OJ) in Ruby last year when their performance benefits were found to be negligible. Well maintained projects use stdlib json. I have encountered memory leaks on OJ on large projects in the past and steer clear of it. It's not cool to force users of this gem to use OJ. The use of rabl and hashie here is unnecessary. Most of the rabl files are identical and very short. Rabl is useful for large schemas and when you need to map to different formats. Why use rabl or hashie when a simple hash will suffice? Rabl also has a hard dependancy on OJ. |
- Add a require for rabl
@john-griffin : A simple hash was initially used, but you can't customize it for your project. If you have any solution for that, I'll be happy to take it :) I'll make some search on OJ to confirm what you say. But well, I made some research last year, and the results was 2-4x faster than the default JSON Parser.
They are mostly the same, but you have to provide a different file for each couple Controller - Action to be able to customize your request. |
@john-griffin - thank you for that link! There is maybe another way to allow for customization of the JSON responses. Right now most of the controller methods look like this: # devise_token_auth/app/controllers/devise_token_auth/registrations_controller.rb
module DeviseTokenAuth
class RegistrationsController < DeviseTokenAuth::ApplicationController
# ...
def update
if @resource
if @resource.update_attributes(account_update_params)
render json: {
status: 'success',
data: @resource.as_json
}
else
render json: {
status: 'error',
errors: @resource.errors
}, status: 403
end
else
render json: {
status: 'error',
errors: ["User not found."]
}, status: 404
end
end
# ...
end
end We could introduce a json rendering method that can be used everywhere, so that in order to customize the responses, only one method would need to be overridden. For example: # devise_token_auth/app/controllers/devise_token_auth/application_controller.rb
module DeviseTokenAuth
class ApplicationController < DeviseController
include DeviseTokenAuth::Concerns::SetUserByToken
respond_to :json
# ...
def render_json_response(data, status, errors)
response = {}
response[:data] = data if data
response[:errors] = errors if errors
status ||= 200
return render json: response, status: status
end
end
end This would DRY up all of the controller methods. For example, the # devise_token_auth/app/controllers/devise_token_auth/registrations_controller.rb
module DeviseTokenAuth
class RegistrationsController < DeviseTokenAuth::ApplicationController
# ...
def update
if @resource
if @resource.update_attributes(account_update_params)
render_json_response(@resource.as_json, 200, null)
else
render_json_response(null, 403, @resource.errors)
end
else
render_json_response(null, 404, ["User not found"])
end
end
# ...
end
end What do you guys think? |
That could be a solution, but what if I don't wan't a |
Then you can override just this one method. We can add an initializer option that allows for custom JSON rendering methods. Something like this: DeviseTokenAuth.setup do |config|
# ...
config.json_response_format_method = MyApp::ApplicationController.render_json_response
# ...
end And then in class ApplicationController < ActionController::Base
# ...
def render_json_response(data, status, errors)
response = {}
response[:alt_data_param] = data if data
response[:alt_error_param] = errors if errors
status ||= 200
return render json: response, status: status
end
# ...
end So you can organize the response however you want. And if you want |
@lynndylanhurley I like that approach, very flexible. |
Looks like a good solution :) And I like this approach too. Very simple, without dependencies. I'll close this PR and make a new one |
@nicolas-besnard - thanks!!! I'll be sure to add you to the contributors list for this 😃 |
Follow of #87 :
I think I'm done. I face a problem where the view is not found :
We can see that the formats of the request if
:html
. I have to force my route to display JSON :