Split out AV from AP, part 2 (WIP) #11396

lukaszx0 · 2013-07-10T15:31:56Z

Hey,

This PR is a continuation of #11032.

I've moved all the rendering-related code to ActionView and added to its railties code that hooks all necessary things to ActionPack if the AV is loaded.

As straight forward as it sounds, it turned out it's not that easy. In the AP, all its components needs to be loaded in order. This introduced huge complication with loading AV stuff in railties as it was included at the end and needed to be included somewhere in the middle.

After discussing this issue with @drogus and @josevalim we agreed on simple solution - https://github.com/strzalek/rails/blob/fc89313fe3f9e508dbe84616fa9a91db123337b0/actionpack/lib/action_controller/base.rb#L164-168

With this code, we're now able to load AV::Rendering between AbstractController::Rendering and ActionPack::Rendering simply by:

ActionPack::Base.superclass.send(:include, ...)

This PR is subject to discussion and WIP, not final solution.

What are your thoughts? Maybe you have other ideas how to abstract ActionView ?

TODOS:

--skip_action_view
changelog
deprecations (if any)
Create BasicRendering implementing only render text:
Create public API method rendering_format in controller
Create ActionPack integration tests for ActionView

/cc @drogus @josevalim @jeremy @wycats @spastorino @tenderlove

josevalim · 2013-07-10T18:33:30Z

actionpack/lib/abstract_controller.rb

@@ -10,12 +10,11 @@ module AbstractController
  autoload :Base
  autoload :Callbacks
  autoload :Collector
+  autoload :DoubleRenderError, "abstract_controller/rendering.rb"


No need for .rb extension.

lukaszx0 · 2013-07-10T22:29:34Z

Thanks @josevalim for the review!

I've fixed the small things. I was aware of moving both view_assigns and protected_vars stuff from AV and I will address them in coming days.

The main purpose of posting this was get opinion on the solution I finally stick to (https://github.com/strzalek/rails/blob/fc89313fe3f9e508dbe84616fa9a91db123337b0/actionpack/lib/action_controller/base.rb#L164-168) from others so please that into account that there's a lot rough edges to polish.

egilburg · 2013-07-10T22:34:52Z

actionpack/lib/action_controller/base.rb

@@ -160,7 +161,11 @@ module ActionController
  #     render action: "overthere" # won't be called if monkeys is nil
  #   end
  #
-  class Base < Metal
+  metal = Class.new(Metal) do


Does this have to be an anonymous class? Would it be easier to debug/extend if it was more explicit?

class MetalWithRendering < Metal include AbstractController::Rendering end class Base < MetalWithRendering

(MetalWithRendering may be a poor name, better ideas welcome)

I would also put some of the explanation in your PR as for the purpose of this class and the way its defined, as code comments describing this class, so people know why this "hack" is needed and why you don't just include Rendering into Base.

I wouldn't make it a named class for now but we can always change it later. Also, you need to add code comments on why we need this class and you need to move the comments already existing to the top of class Base < metal otherwise the documentation will disappear.

lukaszx0 · 2013-07-12T11:01:48Z

@robin850 your comments have been addressed in the latest commit. It's true that it's WIP but it's also very helpful to have those kind of suggestions early in the process. Thank you!

robin850 · 2013-07-12T11:54:12Z

@strzalek : Thanks! I'm always interested in this kind of PR. Glad to help! 😃

carlosantoniodasilva · 2013-07-15T15:37:33Z

actionview/lib/action_view/railtie.rb

+    initializer "action_view.setup_action_pack" do |app|
+      ActiveSupport.on_load(:action_controller) do
+        ActionController::Base.send(:include, ActionView::Layouts)
+      end


You're supposedly already in AC::Base scope under the on_load block, so calling include directly should work I imagine (plus it'd work with rails-api that way).

lukaszx0 · 2013-08-12T15:42:21Z

Few weeks later and it looks like the whole thing is finished! 💥

Tests passes and sample application which doesn't use AV at all can be crated without any problem. You can find it here: https://github.com/strzalek/rails-sandbox

Could you guys give it a look and review all this stuff? It would be very appreciated! ✨

/cc @drogus @josevalim @jeremy

drogus · 2013-08-12T15:48:45Z

I will review the entire PR in the evening, but I just wanted to confirm that I've been able to build simple rails app, which just rendered JSON without AV. The simplest way to do that is to create app with --skip-action-view option. Then you should be able to use render :text in controller in order to render a response.

lukaszx0 · 2013-08-12T15:51:39Z

Right, just like here: https://github.com/strzalek/rails-sandbox/blob/master/app/controllers/pages_controller.rb#L3

guilleiguaran · 2013-08-12T17:37:43Z

@strzalek I'm curious, what's the current status of AV? Is possible to use it without depend on AP? 😃

egilburg · 2013-08-12T18:49:26Z

actionpack/lib/action_controller/metal/rendering.rb

@@ -12,17 +45,17 @@ def process_action(*) #:nodoc:

    # Check for double render errors and set the content_type after rendering.
    def render(*args) #:nodoc:
-      raise ::AbstractController::DoubleRenderError if response_body
+      raise ::AbstractController::DoubleRenderError if self.response_body


What's the need to prefix with self here?

lukaszx0 · 2013-08-12T18:51:16Z

Hah, yes! Using this branch - YES.

The rails-sandbox (https://github.com/strzalek/rails-sandbox) project is rails app without ActionView. It uses rails from my fork and this branch.

You can clone it and give it a try 🎢 !

egilburg · 2013-08-12T18:51:42Z

AV's gemspec does not depend on AP in production, so the short answer is yes.

However, it still has development dependency on AP, which I'm not sure is a good thing - it makes it harder to verify with tests that AV is actually truly independent of AP.

lukaszx0 · 2013-08-12T19:10:57Z

@egilburg it might be possible to remove dev dep from AP on AV but well, I don't think it makes any sense. At the end of the day, AP and AV are still closely tight together and 95% of the project or maybe even more will be always using rails with AV.

However, almost all of the tests concerning ActionView were moved away from AP to AV, and you can find them as integration ones in actionview/test/actionpack (https://github.com/strzalek/rails/tree/extract_renderers/actionview/test/actionpack). Notice that AV has also dev dep on AP as it needs stuff from there.

Besides that, in railties you can find basic_rendering_test.rb (https://github.com/strzalek/rails/blob/extract_renderers/railties/test/application/basic_rendering_test.rb) which is end-to-end test, checking if rails application without AV works as supposed. (it generates full rails app, makes few changes to files and setup, runs it and checks what was rendered).

That's how it is now. I think that AV should stay as dev dep on AP for now. We have quite good coverage to be sure if stuff with/without AV works properly. Maybe it'll change some time in the future, during the process of refactoring tests, because I must admit, in some places you can find huge mess in there.

Hope that explains everything, @egilburg. Let me know if you still have any questions.

guilleiguaran · 2013-08-12T20:57:49Z

@strzalek I was talking about the other use case, using ActionView without Rails, something like this:

https://gist.github.com/drogus/2472931

I'll try it to check if everything works as expected 😄

drogus · 2013-08-12T21:00:18Z

@guilleiguaran this is also possible, see the previous PR #11032 :) @strzalek also confirmed that it works with my gists when you change the Gemfile to include actionview instead of entire actionpack.

guilleiguaran · 2013-08-12T21:02:12Z

@drogus @strzalek nice!!! great work on this guys 😄

lukaszx0 · 2013-08-13T11:34:28Z

Yeah, just like @drogus said, it was possible after previous PR :)

lukaszx0 · 2013-08-13T23:30:46Z

I give a thought about AP and AV as dev dependency and actually it can - and maybe even should - be dropped. I've just pushed 2 commits, they're moving tests from abstract_controller to AV.

At this state, only few small things are keeping AV as dep. I'll try to remove them tomorrow.

HTML Scanner is part of ActionView and it should be loaded along with it

[ci skip]

The ones that were actually testing AV functionality and should belong in there

We're not dealing with templates here [ci skip]

Make it oneliner

lukaszx0 · 2013-08-25T09:53:06Z

Nice tool. I've updated gist with results: https://gist.github.com/strzalek/6293709

I'm again surprised in a good way 😍 I've run tests few times, results are slightly different every time but my branch seems to get either the almost the same numbers as master or lower (which is better).

What do you think 😉 ?

drogus · 2013-08-25T10:07:30Z

actionmailer/actionmailer.gemspec

@@ -20,6 +20,7 @@ Gem::Specification.new do |s|
  s.requirements << 'none'

  s.add_dependency 'actionpack', version
+  s.add_development_dependency 'actionview', version


Have you checked if action mailer actually works without action view? I guess I kind of ignored it when I was doing testing - if it's really as easy to use it without AV as actionpack it's ok to leave this line, otherwise we should either fix it or use add_dependency

drogus · 2013-08-25T10:18:16Z

I gave it another look and apart from one comment about action mailer it looks good to merge. I will be 🚢ing it today in the evening if there is no further concerns from others.

lukaszx0 · 2013-08-26T08:58:26Z

Great catch. That's true, it won't work without ActionView yet. I've pushed commit to make it first class dependency and I think we can merge it.

However I think it'll be easy possible to make ActionMailer work without ActionView to make able sending simple emails, which don't require template rendering (http://guides.rubyonrails.org/action_mailer_basics.html#sending-emails-without-template-rendering), without the need of AV.

I'll work on this later and send another PR not to hold this one.

Remove dependency on Action View from Action Pack This set of changes removes the need of using Action View with Action Pack. Now you may use controllers without Action View, by rendering `:text` response or alternatively you can plug in your own rendering logic. This is especially handy when you're just dealing with APIs and don't need to include entire Action View just to render simple JSON responses.

lukaszx0 · 2013-08-26T10:12:49Z

✨ 😍 🎊

vipulnsward · 2013-08-26T10:14:15Z

👍

lukaszx0 · 2013-08-26T10:18:16Z

For the record. This was quite big change, moving mostly core parts framework which means some issues may arise later on. Feel free to @mention me on github/twitter or mail me directly if there will be any problem caused by this change.

pftg · 2013-08-26T10:30:13Z

👍

spastorino · 2013-08-26T17:00:57Z

@strzalek great work ❤️

egilburg · 2013-08-26T17:16:59Z

Awesome ❤️

I do think that it would be great if development dependencies could be removed between AP and AV. Otherwise, it's easy for someone in a later patch to re-introduce coupling between the two, and the build won't notice.

rafaelfranca · 2013-08-26T17:21:34Z

💚💛❤️💜💙

lukaszx0 · 2013-08-26T19:53:47Z

@egilburg like I've said in previous comments. The goal is get rid of dev-dependencies in the future and I will work on that later. Regarding re-introducing coupling, I think that test added in railties is quite good protection from that.

spastorino · 2013-09-03T20:17:45Z

actionpack/lib/action_controller/metal/rendering.rb

    end

    # Overwrite render_to_string because body can now be set to a rack body.
    def render_to_string(*)
      if self.response_body = super
        string = ""
-        response_body.each { |r| string << r }
+        self.response_body.each { |r| string << r }
        string


This method shouldn't set response_body, I saw it is "reverting" to nil in an ensure block but same could be accomplished with https://gist.github.com/spastorino/19079770e255adc97027.
Even more, shouldn't super already return a string instead of something that returns an object which responds to each?. In that case all the code in this method is useless.

/cc @josevalim @strzalek

Yeah, well to be honest I have no idea what this code does. I've found this commit by @josevalim which introduced it: lukaszx0@4f04452

For me, it also looks like it could've be removed.

Previously, response_body= had a bunch of complicated rules to set the body. So in order to reuse the logic, we assigned the value to response_body which normalized everything to an array. That's probably not the case today and I agree it can be refactored.

Cool. I'll review this then

josevalim reviewed Jul 10, 2013
View changes

egilburg reviewed Jul 10, 2013
View changes

carlosantoniodasilva reviewed Jul 15, 2013
View changes

egilburg reviewed Aug 12, 2013
View changes

lukaszx0 added 11 commits August 25, 2013 11:39

Move AP's capture tests to AV

1e001b1

Basic rendering test

313f1f1

Update AP changelog

44bc45b

Load HTML in ActionView not ActionPack

c40c362

HTML Scanner is part of ActionView and it should be loaded along with it

Extend basic rendering, test it in railties

a2ca04b

Describe BasicRendering shortly

34a2e48

[ci skip]

Move actionpack's controller tests

7620ab0

Move abstract's controller tests to AV

e10a253

The ones that were actually testing AV functionality and should belong in there

Simplify if statements

f6039df

Change description of BasicRendering#render

8c8fb89

We're not dealing with templates here [ci skip]

Fix formatting of error message

ca11cd2

Make it oneliner

drogus reviewed Aug 25, 2013
View changes

Make AV dependency for ActionMailer

0d43df7

drogus merged commit c199580 into rails:master Aug 26, 2013

spastorino reviewed Sep 3, 2013
View changes

robin850 mentioned this pull request Nov 1, 2013

Fix the build rails/coffee-rails#50

Closed

lukaszx0 mentioned this pull request Dec 5, 2013

Retain ActionPack dependency on ActionView #13189

Merged

Split out AV from AP, part 2 (WIP) #11396

Split out AV from AP, part 2 (WIP) #11396

lukaszx0 commented Jul 10, 2013

josevalim Jul 10, 2013

lukaszx0 commented Jul 10, 2013

egilburg Jul 10, 2013

josevalim Jul 12, 2013

lukaszx0 commented Jul 12, 2013

robin850 commented Jul 12, 2013

carlosantoniodasilva Jul 15, 2013

lukaszx0 commented Aug 12, 2013

drogus commented Aug 12, 2013

lukaszx0 commented Aug 12, 2013

guilleiguaran commented Aug 12, 2013

egilburg Aug 12, 2013

lukaszx0 commented Aug 12, 2013

egilburg commented Aug 12, 2013

lukaszx0 commented Aug 12, 2013

guilleiguaran commented Aug 12, 2013

drogus commented Aug 12, 2013

guilleiguaran commented Aug 12, 2013

lukaszx0 commented Aug 13, 2013

lukaszx0 commented Aug 13, 2013

lukaszx0 commented Aug 25, 2013

drogus Aug 25, 2013

drogus commented Aug 25, 2013

lukaszx0 commented Aug 26, 2013

lukaszx0 commented Aug 26, 2013

vipulnsward commented Aug 26, 2013

lukaszx0 commented Aug 26, 2013

pftg commented Aug 26, 2013

spastorino commented Aug 26, 2013

egilburg commented Aug 26, 2013

rafaelfranca commented Aug 26, 2013

lukaszx0 commented Aug 26, 2013

spastorino Sep 3, 2013

lukaszx0 Sep 5, 2013

josevalim Sep 5, 2013

lukaszx0 Sep 5, 2013

Split out AV from AP, part 2 (WIP) #11396

Split out AV from AP, part 2 (WIP) #11396

Conversation

lukaszx0 commented Jul 10, 2013

Choose a reason for hiding this comment

lukaszx0 commented Jul 10, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszx0 commented Jul 12, 2013

robin850 commented Jul 12, 2013

Choose a reason for hiding this comment

lukaszx0 commented Aug 12, 2013

drogus commented Aug 12, 2013

lukaszx0 commented Aug 12, 2013

guilleiguaran commented Aug 12, 2013

Choose a reason for hiding this comment

lukaszx0 commented Aug 12, 2013

egilburg commented Aug 12, 2013

lukaszx0 commented Aug 12, 2013

guilleiguaran commented Aug 12, 2013

drogus commented Aug 12, 2013

guilleiguaran commented Aug 12, 2013

lukaszx0 commented Aug 13, 2013

lukaszx0 commented Aug 13, 2013

lukaszx0 commented Aug 25, 2013

Choose a reason for hiding this comment

drogus commented Aug 25, 2013

lukaszx0 commented Aug 26, 2013

lukaszx0 commented Aug 26, 2013

vipulnsward commented Aug 26, 2013

lukaszx0 commented Aug 26, 2013

pftg commented Aug 26, 2013

spastorino commented Aug 26, 2013

egilburg commented Aug 26, 2013

rafaelfranca commented Aug 26, 2013

lukaszx0 commented Aug 26, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment