-
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
Introduces per-request adapter switching based on the request mime. #2122
base: master
Are you sure you want to change the base?
Conversation
@Fletcher91, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bf4, @domitian and @bacarini to be potential reviewers. |
1309f22
to
c7bf9aa
Compare
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.
IMO, only the documentation here and some tests are required since request.format
is rails-speak for the negotiation mime type, which, for JSON:API is format.jsonapi
, which uses render jsonapi: model
Please feel free to disagree and point out anything I'm not considering
# In controller action | ||
respond_to do |format| | ||
format.json { render json: Model.first } # Uses the JSON adapter | ||
format.json_api { render json: Model.first } # Users the JSON:API adapter |
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.
👍
should be jsonapi
not json_api
, and render jsonapi: model
rather than render json: model
.
But really, this is just using the content type negotiation that respond_to already does. This would work without any changes to the PR, assuming mime -> adapter is 1:1
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.
Would you prefer just a correction:
To automatically switch between adapters based on the request, set the adapter to
:mime
:# In configuration ActiveModelSerializers.config.adapter = :mime # In controller action respond_to do |format| format.json { render json: Model.first } # Uses the JSON adapter format.jsonapi { render jsonapi: Model.first } # Users the JSON:API adapter format.bug { render json: Model.first } # Will raise `UnknownAdapterError` end
or to just leave it implicit:
To automatically switch between adapters based on the request, set the adapter to
:mime
;
ActiveModelSerializers.config.adapter = :mime
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.
Really, if you want to render different mime types, each mime type should be registered to its own formatter. That way, you can do the below, which is the Rails-way IMHO and requires no changes to the codebase.
# In configuration
ActiveModelSerializers.config.adapter = :json
# In controller action
respond_to do |format|
format.json { render json: model, serializer: ModelSerializer } # Uses the JSON adapter
format.jsonapi { render jsonapi: model, serializer: ModelSerializer } # Users the JSON:API adapter
format.bug { render json: model, serializer: ModelSerializer } # Will raise something about bug not being a known mime type
end
The respond_to block does the content type negotiation for you and usually we use a renderer format with the same name as the negotiated / acceptable mime types.
I think I'm making different assumptions than you, so please continue to discuss.
c7bf9aa
to
fdc7a9a
Compare
fdc7a9a
to
a1a78ab
Compare
Purpose
The current behaviour is to always use the same adapter, this is not very desirable in more formal hypermedia conforming systems where content-negotiation determines the format which should be used. This PR introduces the option to let the system choose the adapter based on the negotiation results.
Changes
Content-type determination is left to ActionController::MimeResponds, so that users won't have to specify the adapter in every render call.
Caveats
:mime
to let the subsystem know to look up the adapter dynamically, which may be considered inconsistent.Related GitHub issues
@remear's comment on PR #1082 was the cause to reimplement this feature.
#1039