Skip to content
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

Add support for Rails 5 API-only applications #793

Merged
merged 1 commit into from
Apr 3, 2017

Conversation

codebycliff
Copy link
Collaborator

Description

Draper expects your ApplicationController to include ActionView::Rendering. The
ApplicationController generated by Rails 5 API-only applications (created with
rails new --api) doesn't by default. However, including ActionView::Rendering in
ApplicatonController breaks render :json due to render_to_body being overridden.

This compatibility patch fixes the issue by restoring the original render_to_body
method after including ActionView::Rendering.

Originally, I was going to provide documentation in the README on how to apply this patch. However, I ended up deciding to build it in. That way Draper works out of the box for api only applications at the expense of including ActionView::Rendering in ApplicationController.

Ultimately, including ActionView::Renderingin an ActionController::API may not be supported functionality by Rails (see Rails issue for more detail: rails/rails#27211). This hack is meant to be a temporary solution until we can find a way to not rely on the controller layer.

Testing

  1. Create a new Rails 5 app: rails new api_only --api
  2. Add Draper to the Gemfile: gem 'draper', github: 'drapergem/draper', branch: 'feature/rails-5-api-only-support'
  3. After bundling, run rails c. This should no longer throw an exception.
  4. Generate scaffold with decorator: rails g scaffold Post title description
  5. Verify you can (1) decorate the object and (2) render :json.

To-Dos

  • tests

References

@codebycliff codebycliff added this to the 3.0.0 milestone Apr 1, 2017
@codebycliff
Copy link
Collaborator Author

Does anybody have an opinion on this? @syguer?

@syguer
Copy link
Contributor

syguer commented Apr 3, 2017

@codebycliff
Hmm... I think the important thing now is support rails 5 as soon as possible.
So we have no choice.
Go ahead with this and let's consider about how not be rely on the controller layer after rails 5 supported 👍

@codebycliff codebycliff merged commit 05b107b into master Apr 3, 2017
@codebycliff codebycliff deleted the feature/rails-5-api-only-support branch April 3, 2017 13:19
@ghost
Copy link

ghost commented May 3, 2017

@codebycliff Thanks for working on this! Sorry I haven't been able to respond sooner. I do wonder though, is it possible for Draper to internally use a subclass of ApplicationController by default, so that it's not modifying ApplicationController? Also, would that fix the issue regarding render :json since Draper doesn't call that (I assume). I'm thinking Draper would internally do something like this if default_controller hasn't been explicitly set by the user (untested):

default_controller ||= Class.new(ApplicationController) do
include ActionView::Rendering
end

I haven't looked at the code in a while, so I'm just brainstorming here. Maybe this won't work for reasons I've forgotten.

@franzliedke
Copy link

@codebycliff @syguer To make a long story short, this fix means that I cannot - in a Rails 5 app in API mode - extend from ActionController::API when using both draper and the responders gem. That's due to the latter checking for whether ActionView::Rendering is included in the controller to decide whether to respond with API or default behavior.

That logic might be flawed, but I am unsure how to fix it there.

Since you seem to be intent on removing the "reliance on the controller layer" (as mentioned in the diff), I might be able to help to get this effort started. Is there any place where the plan of attack for that issue is laid out?

@codebycliff
Copy link
Collaborator Author

@jpettettphaxio I agree that it makes more sense to use a subclass and not directly modifying ApplicationController, however, I believe that is a limitation of the architecture. I don't think draper would work anymore because it has to modify the base class that your controllers extend from to inject the behavior. Ultimately, I think it makes more sense to find a way to not rely on the controller layer. Or have that reliance be an opt-in of sorts. Which leads me to the next thing...

@franzliedke I would love to remove the reliance on the controller layer and would appreciate any help you could provide. However, there is no plan of attack at the moment unfortunately. If you have ideas or want to brainstorm solutions, I am all ears. Removing the reliance on the controller would solve your problem above. Without doing that, I am unsure of how much Draper should support ActionController::API considering Rails itself doesn't really support including ActionView::Rendering in ActionController::API yet. I hope that issue gets some traction, but until then I'm not sure of how well supported this behavior will be. Hopefully that makes sense.

@franzliedke
Copy link

@codebycliff Thanks for getting back!

Can you quickly explain what "reliance on the controller layer" currently means? From my quick deep dive, it seems to be about gathering the view and route helpers from the controller, so that they are available in the decorators. Is that right?

@codebycliff
Copy link
Collaborator Author

Yes. There are two files in particular where most of this is happening. In Draper, it's using class_eval to drop in Draper's functionality and a before_action callback on the controllers. This callback is responsible for "saving" the controller instance in Draper::ViewContext, as well as overriding ActionController#view_context to "save" the controller's ViewContext. There are a couple more files that also deal with the controller layer, but that should get you an idea of how it works.

zdavis added a commit to ManifoldScholar/manifold that referenced this pull request Dec 13, 2018
When we installed Draper to provide decorators to our mailers, we
inadvertently broke the OAuth controller. This is likely due to the
following issues in Draper, which do not appear to have been fully
solved in Draper 3.0.1.

Because this is the only place we use ActionView outside of Mailers,
for now we will just render the content from the controller. In the
future, if we need more view functionality, we'll use cells.

See:
rails/rails#27211
drapergem/draper#793

Fixes #1631
SMaxOwok pushed a commit to ManifoldScholar/manifold that referenced this pull request Dec 13, 2018
When we installed Draper to provide decorators to our mailers, we
inadvertently broke the OAuth controller. This is likely due to the
following issues in Draper, which do not appear to have been fully
solved in Draper 3.0.1.

Because this is the only place we use ActionView outside of Mailers,
for now we will just render the content from the controller. In the
future, if we need more view functionality, we'll use cells.

See:
rails/rails#27211
drapergem/draper#793

Fixes #1631
zdavis added a commit to ManifoldScholar/manifold that referenced this pull request Dec 14, 2018
When we installed Draper to provide decorators to our mailers, we
inadvertently broke the OAuth controller. This is likely due to the
following issues in Draper, which do not appear to have been fully
solved in Draper 3.0.1.

Because this is the only place we use ActionView outside of Mailers,
for now we will just render the content from the controller. In the
future, if we need more view functionality, we'll use cells.

See:
rails/rails#27211
drapergem/draper#793

Fixes #1631
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants