Skip to content

Loading…

Should not eager_load app/assets #7587

Merged
merged 3 commits into from

7 participants

@elia

If I happen to use a .rb file inside app/assets it can get automatically loaded, this probably holds true for app/views.

I think this is not the intended behavior, railties has:

paths.add "app",                 :eager_load => true, :glob => "*"
paths.add "app/assets",          :glob => "*"
paths.add "app/controllers",     :eager_load => true
paths.add "app/helpers",         :eager_load => true
paths.add "app/models",          :eager_load => true
paths.add "app/mailers",         :eager_load => true
paths.add "app/views"

seems quite clear that the first line refers to all non-specified paths

Of course I can fix this per application by adding to application.rb:

    config.before_initialize do
      config.eager_load_paths = config.eager_load_paths.dup - Dir["#{Rails.root}/app/{assets,views}"]
    end
@senny
Ruby on Rails member

@rafaelfranca, @steveklabnik is this intended or a bug?

@steveklabnik
Ruby on Rails member

I hve no idea, to be honest.

@senny
Ruby on Rails member

@steveklabnik can you get someone to decide how we should proceed? I'll look into the issue if it is a bug but I think we should confirm that first.

@steveklabnik
Ruby on Rails member

Working on it.

@guilleiguaran
Ruby on Rails member
@elia

Just to help deciding :)

Can you think of any legit use case in which someone can put non-asset stuff inside app/assets or non-view stuff inside app/views?

@guilleiguaran
Ruby on Rails member

@elia mustache .rb views in app/views?

@elia

@guilleiguaran right, but I still feel that should be mustache-rails responsibility to add app/views to the load path, and add app/templates to the views path. That is what they do already

@guilleiguaran
Ruby on Rails member

@elia agree

@rafaelfranca
Ruby on Rails member

Looking at the code I see that these lines that you pointed out are not causing this issue since add passes the options to the Path.new and Path.new only mark the path as eager loaded if the options is present.

I'm trying to find the spot, but would be nice an example application reproducing the issue or a failing test.

@rafaelfranca
Ruby on Rails member

just did a failing test case:

diff --git a/railties/test/application/paths_test.rb b/railties/test/application/paths_test.rb
index 4029984..f85c284 100644
--- a/railties/test/application/paths_test.rb
+++ b/railties/test/application/paths_test.rb
@@ -59,6 +59,8 @@ module ApplicationTests
       assert eager_load.include?(root("app/controllers"))
       assert eager_load.include?(root("app/helpers"))
       assert eager_load.include?(root("app/models"))
+      assert !eager_load.include?(root("app/views")), "expected to not be in the eager_load_path"
+      assert !eager_load.include?(root("app/assets")), "expected to not be in the eager_load_path"
     end

     test "environments has a glob equal to the current environment" do
@@ -73,6 +75,7 @@ module ApplicationTests
       assert_in_load_path "vendor"

       assert_not_in_load_path "app", "views"
+      assert_not_in_load_path "app", "assets"
       assert_not_in_load_path "config"
       assert_not_in_load_path "config", "locales"
       assert_not_in_load_path "config", "environments"
@rafaelfranca
Ruby on Rails member

I found the spot too. It is the :glob option.

@elia

@rafaelfranca good, if it's agreed to fix this I can submit a patch

@rafaelfranca
Ruby on Rails member

Yes, we need to fix this. It is not intended.

@rafaelfranca
Ruby on Rails member
@josevalim
Ruby on Rails member

Yes, this needs to be fixed. This is the proper fix imo:

paths.add "app",                 :eager_load => true, :glob => "*"
paths.add "app/assets",          :eager_load => false, :glob => "*"
paths.add "app/views",           :eager_load => false

If this syntax does not work, we should make it work.

@elia

Promoted to pull-request, the last commit is slightly off-topic, let me know if I need to ditch it

For the record the whole came up using opal-rails which has support for app/assets/javascripts/application.js.rb

@rafaelfranca
Ruby on Rails member

@elia great!!

Could you add a test to the case in the last commit?

Also we'll need a CHANGELOG entry

@elia

@rafaelfranca was looking for a user of Path#children in order to add the test but seems unused… should I remove or deprecate?

@elia elia Respect children paths filter settings
E.g. don't eager-load app/assets even if app/* has the eager_load flag set.
53778ec
@rafaelfranca
Ruby on Rails member

Since it is public API I would deprecate.

@elia

@rafaelfranca ok, should I also revert the change? rails won't benefit from it, but can possibly break external code relying on #children

Another question: it's desired to have so heavy integration tests instead of some lightweight units?
(this one I used while developing this patch)

@rafaelfranca
Ruby on Rails member

@elia yes. Please revert the changes.

I don't like the idea to have unit and integration tests asserting the same thing. Since we need to assert the behavior of the railties with integration tests I don't think we should have unit tests too.

Also these tests are weak because I can change the paths in rails/engine/configuration.rb and forget to change in that before block.

@elia

@rafaelfranca deprecated and removed the commit, we should be ok.

Thanks for the explanation, I'm all for giving priority to integration tests, the issue is that they're not quick enough during TDD (never thought of actually using that in the gist btw :smile:).

@rafaelfranca rafaelfranca merged commit a53e464 into rails:3-2-stable
@rafaelfranca
Ruby on Rails member

Thank you

@rafaelfranca rafaelfranca added a commit that referenced this pull request
@rafaelfranca rafaelfranca Revert "Merge pull request #7587 from elia/fix-too-eager-loading"
This reverts commit 3663057.

REASON: This caused a regression that add app folder in the eager load
path. See #8146 for more information.

Conflicts:
	railties/CHANGELOG.md
7f96e43
@chancancode
Ruby on Rails member

@elia do you know what's the current state of this? I tried to look through the related tickets but I'm still a bit confused.

As far as I can tell, this is what happened:

  1. We confirmed that we do not intend for /view and /assets to be eager loaded
  2. The patch was accepted
  3. The patch had an unintended side-effect (adding /app to the eager load path) and caused a regression, so it was reverted until we find a proper fix that would avoid the side-effect
  4. ???

Are you saying that we the original bug (/view and /assets are eager loaded) is still open? In that case we "just" need to figure out why this patch was causing /app to be added and avoid that, so we can re-apply the patch

@elia

"4." is just me not having time to dig deeper and fix it :disappointed:

I guess the bug it's still there but no one noticed, given the fact that is pretty rare to have .rb files in the assets folder. Unless of course if you're using @opal. But even in that case there's that "nice" hack in opal-rails that prevents the bug from surfacing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 27, 2012
  1. @rafaelfranca @elia
  2. @elia

    Respect children paths filter settings

    elia committed
    E.g. don't eager-load app/assets even if app/* has the eager_load flag set.
Commits on Oct 29, 2012
  1. @elia
This page is out of date. Refresh to see the latest.
Showing with 17 additions and 6 deletions.
  1. +2 −0 railties/CHANGELOG.md
  2. +12 −6 railties/lib/rails/paths.rb
  3. +3 −0 railties/test/application/paths_test.rb
View
2 railties/CHANGELOG.md
@@ -1,5 +1,7 @@
## Rails 3.2.9 (unreleased)
+* Don't eager-load app/assets and app/views *Elia Schito*
+
* Update supported ruby versions error message in ruby_version_check.rb *Lihan Li*
## Rails 3.2.8 (Aug 9, 2012) ##
View
18 railties/lib/rails/paths.rb
@@ -87,14 +87,15 @@ def load_paths
protected
def filter_by(constraint)
- all = []
+ yes = []
+ no = []
+
all_paths.each do |path|
- if path.send(constraint)
- paths = path.existent
- paths -= path.children.map { |p| p.send(constraint) ? [] : p.existent }.flatten
- all.concat(paths)
- end
+ paths = path.existent + path.existent_base_paths
+ path.send(constraint) ? yes.concat(paths) : no.concat(paths)
end
+
+ all = yes - no
all.uniq!
all
end
@@ -123,6 +124,7 @@ def children
keys.delete(@current)
@root.values_at(*keys.sort)
end
+ deprecate :children
def first
expanded.first
@@ -194,6 +196,10 @@ def existent_directories
expanded.select { |d| File.directory?(d) }
end
+ def existent_base_paths
+ map { |p| File.expand_path(p, @root.path) }.select{ |f| File.exist? f }
+ end
+
alias to_a expanded
private
View
3 railties/test/application/paths_test.rb
@@ -61,6 +61,8 @@ def assert_not_in_load_path(*path)
assert eager_load.include?(root("app/controllers"))
assert eager_load.include?(root("app/helpers"))
assert eager_load.include?(root("app/models"))
+ assert !eager_load.include?(root("app/views")), "expected to not be in the eager_load_path"
+ assert !eager_load.include?(root("app/assets")), "expected to not be in the eager_load_path"
end
test "environments has a glob equal to the current environment" do
@@ -75,6 +77,7 @@ def assert_not_in_load_path(*path)
assert_in_load_path "vendor"
assert_not_in_load_path "app", "views"
+ assert_not_in_load_path "app", "assets"
assert_not_in_load_path "config"
assert_not_in_load_path "config", "locales"
assert_not_in_load_path "config", "environments"
Something went wrong with that request. Please try again.